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 573718 - Change g_timeout_adds to g_timeout_add_seconds to consolidate wakeups
Change g_timeout_adds to g_timeout_add_seconds to consolidate wakeups
Product: metacity
Classification: Other
Component: general
Other Linux
: Normal normal
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Reported: 2009-03-02 07:24 UTC by gQuigs
Modified: 2009-03-06 22:52 UTC
See Also:
GNOME target: ---
GNOME version: ---

wakeups should be less evil (2.23 KB, patch)
2009-03-02 07:25 UTC, gQuigs
none Details | Review
Use seconds instead (1.45 KB, patch)
2009-03-03 02:17 UTC, gQuigs
none Details | Review

Description gQuigs 2009-03-02 07:24:46 UTC
Inspired from

Patch below, All are equivalent except they should consolidate wakeups except for the PING_TIMEOUT_DELAY which I increased from 2.25 seconds to 3.
Comment 1 gQuigs 2009-03-02 07:25:37 UTC
Created attachment 129831 [details] [review]
wakeups should be less evil
Comment 2 Havoc Pennington 2009-03-03 00:15:09 UTC
None of these are "repeated" timeouts that matter for wakeups; they are one-shot timeouts. Changing to add_seconds is probably bad in the ping case, where it is a user experience issue how long that timeout lasts. add_seconds both makes the length nondeterministic (since it has to snap to next even second) and changes it from 2.25 to 3.
Comment 3 gQuigs 2009-03-03 02:17:12 UTC
Created attachment 129912 [details] [review]
Use seconds instead

Removed the ping one, as long as the others don't need the accuracy I still think switching to seconds make sense.  Thanks.
Comment 4 Havoc Pennington 2009-03-04 15:28:21 UTC
> think switching to seconds make sense

why though?

The glib docs say:
  If you want to have a timer in the "seconds" range and do not care about  
  the exact time of the first call of the timer, use the
  g_timeout_add_seconds() function
  To allow this grouping, the interval to the first timer is rounded and can
  deviate up to one second from the specified interval.

In these one-shot timeout cases, we care *only* about the time to the first call, and since the interval is only 1 or 2 seconds to begin with, a 1-second deviation can eliminate the timeout entirely!

Not that it's likely to matter that much either way in practice, but to the extent it does, add_seconds() seems clearly wrong.

The bottom line the Ubuntu page seems to encourage a global s/timeout_add/timeout_add_seconds/ but that is just wrong. The purpose of timeout_add_seconds is to deal with *recurring* timeouts. Otherwise, timeout_add_seconds just messes things up, especially for a 1-second-or-less one-shot timeout.
Comment 5 gQuigs 2009-03-06 03:25:50 UTC
Thanks for the explanation. (Closing Bug)

It just looked to me like a easy way to start contributing, guess it's more complicated than it seemed on that page.  

Places that use 3000 or more should be OK candidates, right? (none in metacity)
Comment 6 Havoc Pennington 2009-03-06 22:52:11 UTC
yep thanks for the patch of course, I just think the ubuntu page needs to explain a bit more.

I think add_seconds should be used when:

1) the timeout is at least 1 second (and can be a round number of seconds)
2) the timeout is recurring i.e. happens over and over in the background

If the timeout callback returns "TRUE" that keeps the timeout installed; if "FALSE" that removes the timeout. So if a timeout returns FALSE, usually it is not a recurring timeout.