Created attachment 193772 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

Also note that monotonic time does not include time spent while
suspended (at least on Linux).

Created attachment 193773 [details] [review]
gmain: Add g_timeout_real_time_source_new()

Several different codebases in GNOME want to implement wall clocks.
While we could pretty easily share a private library, it's not a
substantial amount of code, and GLib already has a lot of the
necessary system-specific detection and handling infrastructure.

Note this initial implementation just wakes up once a second; we'll
add the Linux-specific handling in a subsequent commit.

Created attachment 193774 [details] [review]
Add Linux timerfd_create() backend for g_timeout_real_time_source_new()

This makes the source efficient on Linux.

Untested!  TFD_TIMER_CANCEL_ON_SET was only merged after 2.6.28.

Review of attachment 193773 [details] [review]:

::: glib/gmain.c
@@ +4190,3 @@
+ * @function: function to call
+ * @data:     data to pass to @function
+ * @notify:   function to call when the timeout is removed, or %NULL

A rare case: too much docs... the function only has one parameter.

@@ +4198,3 @@
+ *
+ * Note that the timeout will fire as soon as the specified wall clock
+ * time has expired, or the system clock is reset in any way (forwards

Maybe 'changed' is clearer than 'reset' here ?

@@ +4203,3 @@
+ * For example, this function allows implementing a HOUR:MINUTE clock
+ * display in an efficient way.  In this case, you would want to
+ * specify an @interval of 60.

Might be good to turn this into an actual example - that would also show that you have to get the new time to display in the callback, instead of relying on the interval.

::: glib/tests/timeout.c
@@ +103,3 @@
+ * we would have some way to test the behavior when the system clock
+ * changes, but that's hard to do without requiring root privileges
+ * and using system-specific interfaces.

Won't this test also fail in the absence of timerfd support ?
Because then you'll fall back to waking up every second...

@@ +124,3 @@
+
+  realtime_cur = g_get_real_time ();
+

I think you also want to g_assert (fired) ?

Created attachment 193911 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

No changes, reattaching for ordering

Created attachment 193912 [details] [review]
gmain: Add g_timeout_real_time_source_new()

Update docs and test for matthias' suggestions

Created attachment 193913 [details] [review]
Add Linux timerfd_create() backend for g_timeout_real_time_source_new()

Bugs fixed - tested and working now.

Created attachment 193914 [details] [review]
g_timeout_real_time_add_full: New convenience wrapper

Originally I wasn't planning to offer the full suite of wrappers for
_source_new().  However, since it's trivial to do this one, and it turns
out that since GSource isn't presently introspecteable (thus requiring
a C wrapper in gnome-shell), let's just do it here.

Actually while porting gnome-screensaver and gnome-shell I realized this interface is suboptimal for a few reasons:

1) We should make it explicitly a one-shot callback, since the point of the interface is you may be woken up early, so each time you need to recompute the timeout
2) There's a race condition in the timerfd code where if your app does:

  * Update display
  * Rearm timeout

If the clock is set backwards in between the two, then since the function re-gets the current wall clock time and queues an offset based on that, we'll be waiting a long time to update it.  I think the function needs to take the displayed wall clock time as a parameter.


Actually I'm thinking now I'll go with Ray's original suggestion and make g_date_time_create_watch().

(In reply to comment #9)
> 1) We should make it explicitly a one-shot callback, since the point of the
> interface is you may be woken up early, so each time you need to recompute the
> timeout

If we're trying to reduce duplicated code, then don't make the caller recompute it. The source should do it itself. Just tell it how often you want to be woken up (every second, every minute, every hour, etc), and it should take care of everything for you.

(In reply to comment #9)
> Actually while porting gnome-screensaver and gnome-shell I realized this
> interface is suboptimal for a few reasons:
> 
> 1) We should make it explicitly a one-shot callback, since the point of the
> interface is you may be woken up early, so each time you need to recompute the
> timeout
> 2) There's a race condition in the timerfd code where if your app does:
> 
>   * Update display
>   * Rearm timeout
> 
> If the clock is set backwards in between the two, then since the function
> re-gets the current wall clock time and queues an offset based on that, we'll
> be waiting a long time to update it.
Oh, I just mentioned this in bug 656627. Guess i should have come here first.

> Actually I'm thinking now I'll go with Ray's original suggestion and make
> g_date_time_create_watch().
If you go with that approach then it could monitor timezone changes, under the hood, too.

(In reply to comment #10)
> If we're trying to reduce duplicated code, then don't make the caller recompute
> it. The source should do it itself.

Or maybe better, have a g_real_timeout_add() that does realtime timeouts, and then the clock-specific source could use g_source_add_child_source() to attach one of those to itself.

(In reply to comment #10)
> (In reply to comment #9)
> > 1) We should make it explicitly a one-shot callback, since the point of the
> > interface is you may be woken up early, so each time you need to recompute the
> > timeout
> 
> If we're trying to reduce duplicated code, then don't make the caller recompute
> it. The source should do it itself. Just tell it how often you want to be woken
> up (every second, every minute, every hour, etc), and it should take care of
> everything for you.

Well, there is also the "alarm" case that Evolution has.  So we need both one-shot and periodic.

I think it'll work out OK if we make the clock be an alarm re-set for every minute.

Review of attachment 193911 [details] [review]:

Looks good

Created attachment 194108 [details] [review]
Add g_date_time_create_watch()

Created attachment 194109 [details] [review]
Add Linux timerfd backend

Created attachment 194110 [details] [review]
GTimeZoneMonitor patch

This is looking better; note I now expose cancel_on_set as well as making the source explicitly one shot.  The GDateTime is now the implicit target time; you don't need to calculate an offset.

Note though to really fulfill the API contract, I need to detect time zone changes in the cancel_on_set case.  Right now GDateTime lives in glib/, but GTimeZoneMonitor is in gio/ because that's where the file monitoring bits are =(

GTimeZoneMonitor is a really lame class honestly.  I think we should figure out some change where new GDateTime instances pick up the new timezone automatically, at least for Linux.  We could write some duplicate inotify code for glib/.  

A much more elaborate change would be to try pulling the file monitoring code down into glib/, perhaps as an underscore-prefixed _g_private_file_monitor or something.

Review of attachment 194108 [details] [review]:

::: glib/gdatetime.c
@@ +2646,3 @@
+ * g_date_time_create_watch:
+ * @datetime: Time to await
+ * @cancel_on_set: Also invoke callback if the system clock changes discontiguously

cancel_on_set seems like a fairly nondescriptive name for this parameter. In particular the 'cancel' part. Maybe something like trigger_on_clock_set instead ?

Review of attachment 194108 [details] [review]:

I would still like to see a real example in the docs.

Comment on attachment 193911 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

Attachment 193911 [details] pushed as f0db0d2 - gmain: Clarify that timeouts are in terms of monotonic time

Created attachment 194357 [details] [review]
gdatetime: Add g_date_time_create_watch()

* Add example
* Update gtk-doc metadata
* Update public symbols lists

Created attachment 194358 [details] [review]
Add Linux timerfd_create() backend for g_date_time_create_watch()

No real changes; reattaching for ordering

Created attachment 194359 [details] [review]
GTimeZoneMonitor: Revert addition of this class

The main rationale for adding it was to avoid having gnome-shell
mmap'ing /etc/localtime once a second.  However, we can just as easily
run inotify there, and given no one else was clamoring for a way to
detect when the time zone changes, I don't see a need for public API
here - at least not yet.

In the bigger picture, I just don't believe that the vast majority of
applications are going to go out of their way to instantiate and keep
around a random GTimeZoneMonitor class.  And if they do, it's has the
side effect that for other bits of code in the process, local GDateTime
instances may start varying again!

So, if code can't rely on local GDateTime instances being in a
consistent state anyways, let's just do that always.  The
documentation now says that this is the case.  Applications have
always been able to work in a consistent local time zone by
instantiating a zone and then using it for GDateTime constructors.

We fix the "gnome-shell stats /etc/localtime once a second" issue by
using timerfd (in glib) and inotify (in gnome-shell).

(In reply to comment #19)
> Review of attachment 194108 [details] [review]:
> 
> ::: glib/gdatetime.c
> @@ +2646,3 @@
> + * g_date_time_create_watch:
> + * @datetime: Time to await
> + * @cancel_on_set: Also invoke callback if the system clock changes
> discontiguously
> 
> cancel_on_set seems like a fairly nondescriptive name for this parameter. In
> particular the 'cancel' part. Maybe something like trigger_on_clock_set instead

I chose the name for consistency with the Linux timerfd_settime() flag.  Thought about changing it, but I like the consistency and the docs make pretty clear what it does.

(In reply to comment #20)
> Review of attachment 194108 [details] [review]:
> 
> I would still like to see a real example in the docs.

Done.

GTimeZoneMonitor was only ever added as a kludge because gnome-shell wanted such a thing.  Since it's not even used there at all, I have zero attachment to it.

Rip it out :)

about the API addition, I really have just two comments:

first, the API should be presented as a new GMainContext source in the usual way -- not as part of GDateTime's API.

second, we should really add the timezone-change facilities here so that you have one-stop shopping for detecting any changes in the local time.  Colin suggested that we could use inotify on Linux and fallback to one-per-second polling on other OSes (since we already do that for lack of timerfd).  That makes me cringe, but I'd consider it to be acceptable.

(In reply to comment #28)
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.

...

> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux

if we aren't making it part of the GDateTime API, then couldn't we just put the whole thing in gio, and then use GFileMonitor?

(In reply to comment #29)
> 
> if we aren't making it part of the GDateTime API, then couldn't we just put the
> whole thing in gio, and then use GFileMonitor?

Where would it go in gio?  A new GWallClock class?  Kind of ugh =(

I don't have a really strong opinion on whether it's 

g_date_time_create_watch() in gdatetime.h (modeling on g_io_create_watch())
or
g_date_time_source_new() in gmain.h (modeling on g_timeout_source_new())

Comment on attachment 194359 [details] [review]
GTimeZoneMonitor: Revert addition of this class

Attachment 194359 [details] pushed as 5b68b49 - GTimeZoneMonitor: Revert addition of this class

(In reply to comment #28)
> about the API addition, I really have just two comments:
> 
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.

I modeled it after g_io_create_watch().  Do you think it's a bad example?

Side note: the whole "watch" versus "source" terminology is super confusing; I ended up deciding that "source" is right, and only used _watch in the symbol name for consistency with g_io_create_watch().

Maybe we can call it g_date_time_source_new() and keep it in gdatetime.h?

> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux and fallback to one-per-second
> polling on other OSes (since we already do that for lack of timerfd).  That
> makes me cringe, but I'd consider it to be acceptable.

Yeah.

(In reply to comment #28)
> about the API addition, I really have just two comments:
> 
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.
> 
> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux and fallback to one-per-second
> polling on other OSes (since we already do that for lack of timerfd).  That
> makes me cringe, but I'd consider it to be acceptable.

Sounds acceptable to me too, in the short term. When glib collapses into a single shared library, we can make it use a file monitor...

irc discussion about the name question seems to point towards

g_date_time_create_source
g_date_time_add_watch
g_date_time_add_watch_full

(In reply to comment #34)
> irc discussion about the name question seems to point towards
> 
> g_date_time_create_source

I went with this.

> g_date_time_add_watch
> g_date_time_add_watch_full

We can add these later if someone cares - but the users for this are pretty limited (wall clock implementors and evolution basically).

There is a race here:

Because the user provides the time for themselves and because we are not monitoring for time discontinuities yet, there could be a discontinuity that exists between the time the user acquires the time and the time that the timerfd is actually setup.


Say it's 5:00 by the system time.  We display the time as 5:00, then ask to be woken at 5:01.  While we were doing that, something set the system time back to 4:00.  We've still asked to be woken at 5:01, so it will be 61 minutes before we get called.

Created attachment 195195 [details] [review]
proposed alternate API

we can solve the problem i mention in the previous comment only with a real periodic clock API

any "wake me up at this time" API will be subject to the race...

(In reply to comment #36)
> There is a race here:
> 
> Because the user provides the time for themselves and because we are not
> monitoring for time discontinuities yet, there could be a discontinuity that
> exists between the time the user acquires the time and the time that the
> timerfd is actually setup.

Hm, true.

CPU1:                                                              CPU2:
clock_gettime(CLOCK_REALTIME)
timerfd_create()
                                                                         settimeofday()
timerfd_settime(TFD_CANCEL_ON_SET)


My initial reaction is that we need to take the current time as an argument.  And if we're doing that, we might as well explicitly separate the clock vs alarm cases as you were arguing.

So a new API proposal might be:

GSource *               g_date_time_clock_source_new                    (GDateTime      *now,
                                                                                                       GDateTime      *expiry);

And the example clock:

static gboolean
redisplay_clock (gpointer data)
{
  GSource *source;
  GDateTime *now, *expiry;

  now = g_date_time_new_now_local ();

  g_print ("%02d:%02d\n",
	   g_date_time_get_hour (now),
	   g_date_time_get_minute (now));

  expiry = g_date_time_add_seconds (now, 60 - g_date_time_get_second (now));
  source = g_date_time_clock_source_new (now, expiry);
  g_source_set_callback (source, redisplay_clock, NULL, NULL);
  g_source_attach (source, NULL);
  g_source_unref (source);

  g_date_time_unref (expiry);
  g_date_time_unref (now);

  return FALSE;
}

Created attachment 195202 [details] [review]
g_get_monotonic_time: fix race condition

(noticed while skimming through the code)

Review of attachment 195195 [details] [review]:

::: glib/gmain.c
@@ +5042,3 @@
+ * Among them: progression of monotonic time, detected time
+ * discontinuities, detected timezone change or the passage of 1 second
+ * on non-Linux platforms (ie: we have to check manually every second).

I think the 'or the passage of 1 second...' part is obscuring things a bit.

You should separate the description of what events we _want_ to wake up for
from the description of how often platform inadaequacies force us to wake up.

@@ +5047,3 @@
+ * 'ready'.  That allows us the possibility of using child sources for
+ * some events (although we do not currently do so).  Of course, this
+ * means that we'll get a lot of spurious dispatches.

This should maybe say 'we may get some spurious dispatches' - after all, the goal of all of this is to wake up as rarely as possible, right ?

@@ +5291,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

allow-none ?

@@ +5304,3 @@
+ * specified granularity are ignored, but all other changes result in
+ * @function being called.  The call occurs as close after the "top" of
+ * the interval as possible.

But we are not limited to the predefined timespans here, right ? eg it would work fine to use 30*G_TIME_SPAN_SECOND to update a clock every half minute ?
Might be good to clarify that

@@ +5326,3 @@
+ * changes in the time zone or due to a laptop being suspended and
+ * resumed.  In these cases, the function will be called as soon as
+ * possible.

'not at the top' might also happen due to long-running callbacks of higher priority, I guess.

@@ +5332,3 @@
+ * considered to have been created.  This is useful for avoiding race
+ * conditions that might be associated with obtaining a second copy of
+ * the current time for yourself.

I think @now needs some more explanation - what race is there ? the only danger is that you might miss the first tick ?

@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

allow-none?

@@ +5369,3 @@
+ *                              source was created
+ * @function: the #GRealTimeoutFunc to call
+ * @user_data: a pointer to pass to @function

or NULL

::: glib/gmain.h
@@ +159,3 @@
+typedef gboolean (*GRealTimeoutFunc) (GDateTime *datetime,
+                                      gpointer   data);
+

This seems problematic to me - all other sources we have use GSourceFuncs, so you are going to have to do icky casting, and if I just create such a source and use a source func as callback, its not going to work as expected.

::: glib/tests/glib-clock.c
@@ +13,1 @@
+  return TRUE;

The api is clearly designed to make this demo look good :-)

Review of attachment 195202 [details] [review]:

::: glib/gmain.c
@@ +2056,3 @@
+	  g_once_init_leave (&clockid, (gsize)CLOCK_MONOTONIC);
+	else
+	  g_once_init_leave (&clockid, (gsize)CLOCK_REALTIME);

I'd prefer to have a single leave call to match the enter call. Use a temporary ?

Review of attachment 195195 [details] [review]:

So...this moves the API around quite a bit.  Before we go farther, what do you think about the approach of tweaking the current API (make it take the current time as a parameter, be clock-only, drop @cancel_on_set)?

::: glib/tests/glib-clock.c
@@ +9,3 @@
+           g_date_time_get_minute (now),
+           g_date_time_get_second (now),
+           g_date_time_get_microsecond (now));

Every 5 seconds?  It'd be clearer if we preserved the clock only updating once a minute.

Or if you want to be a bit fancier, take a --seconds argument or something.

Review of attachment 195195 [details] [review]:

Will adjust comments/docs as well.

::: glib/gmain.c
@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

don't out parameters always implicitly allow NULL?  i'd actually expect that 'allow-none' means that the *returned* value could possibly be NULL.

::: glib/gmain.h
@@ +159,3 @@
+typedef gboolean (*GRealTimeoutFunc) (GDateTime *datetime,
+                                      gpointer   data);
+

We have GCancellableSourceFunc, GSocketSourceFunc, GChildWatchFunc, among others.

::: glib/tests/glib-clock.c
@@ +13,1 @@
+  return TRUE;

I consider this demo to be an extremely typical clock.  We should try to make the typical case easy. :)

Review of attachment 195195 [details] [review]:

You should probably split the g_date_time_source_new-removal off into a separate patch.

Review of attachment 195202 [details] [review]:

With that change, it looks fine to commit.

Created attachment 195248 [details] [review]
g_get_monotonic_time: fix race condition

updated for Matthias's suggestion

Created attachment 195252 [details] [review]
g_date_time_source_new: Fix a race condition

Ryan Lortie pointed out that we weren't handling the case where the
clock changed between getting the current time and creating a timerfd.
Change the API to accept the current time too.

I didn't actually attempt to reproduce the race condition, but the above is the relatively obvious incremental patch on top of the existing API.

Notable points versus Ryan's:

1) We still attempt to handle the calendaring/event case
2) It's a pretty incremental change and mostly inherits the testing I already did

Review of attachment 195195 [details] [review]:

::: glib/gmain.c
@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

In most places, we deal with NULL to mean 'not interested in this', but we typically document that fact.

So after more IRC discussion, options:

1) Apply my patch on top of what we have now
2) Rip it out and put it in libgnome-desktop
3) Apply desrt's patch

I've listed this in my personal order of preference roughly.  I'm fine with mclasen picking one of these options.

If 3) is chosen, I'd like to see it be two separate patches as mentioned earlier (revert, then add new API)

Review of attachment 195248 [details] [review]:

thanks, looks good now

Comment on attachment 195248 [details] [review]
g_get_monotonic_time: fix race condition

Attachment 195248 [details] pushed as 2955981 - g_get_monotonic_time: fix race condition

Created attachment 195268 [details] [review]
Revert "gdatetime: Add g_date_time_source_new()"

This reverts three commits:

 - 1feb752996b404965a2f58b29a569a273d4374fa
 - 5763c631473539746646697e6a775f6eacaa08e2
 - 21a538934091e1449e0479daf066fa20df2dc2ef

Created attachment 195269 [details] [review]
New GTimeout.add_real() API

Introduce an API to simplify the needs of applications wanting to
periodically update a clock being displayed to the user (and other
similar needs).

Here's the patch that we will need for both choices 2 and 3 and a work-in-progress patch if we decide to go with #3.

Compared to the last iteration this patch introduces support for detecting changes in the timezone.  There are some TODOs still, and I haven't finished incorporating suggestions from the previous review.

(In reply to comment #54)
> Created an attachment (id=195269) [details] [review]
> New GTimeout.add_real() API
> 
> Introduce an API to simplify the needs of applications wanting to
> periodically update a clock being displayed to the user (and other
> similar needs).

Are there any similar needs that aren't actually wall clock displays?

I can't think of any off the top of my head that couldn't be served equally well (or better) with a monotonic timer.

Comment on attachment 195268 [details] [review]
Revert "gdatetime: Add g_date_time_source_new()"

After consulting with Matthias, I've backed out the new API addition while still keeping GTimeZoneMonitor dead.  The new API will also not be added.

We can revisit this at the start of next cycle.

(In reply to comment #57)
> I can't think of any off the top of my head that couldn't be served equally
> well (or better) with a monotonic timer.

Then the whole g_real_time stuff seems like a bad name - if it's for wall clocks, make it g_wall_clock_source_new ()?

(In reply to comment #58)
> (From update of attachment 195268 [details] [review])
> After consulting with Matthias, I've backed out the new API addition while
> still keeping GTimeZoneMonitor dead.  The new API will also not be added.
> 
> We can revisit this at the start of next cycle.

Sounds fine to me.  In the meantime I have to fix gnome-screensaver; what I'm doing now is changing the existing gnome-tz-monitor.h in gnome-desktop to be a gnome-wall-clock.h.

(In reply to comment #59)
> Sounds fine to me.  In the meantime I have to fix gnome-screensaver; what I'm
> doing now is changing the existing gnome-tz-monitor.h in gnome-desktop to be a
> gnome-wall-clock.h.

You mean the gnome-tz-monitor I removed because nobody used it?
http://git.gnome.org/browse/gnome-desktop/commit/?id=152735fcc4921c83217ab735c316d26bb473539b

GnomeWallClock now lives in https://bugzilla.gnome.org/show_bug.cgi?id=657955

Any news here? Will GDateTimeSource get introspection?

(In reply to comment #62)
> Will GDateTimeSource get introspection?

Oops, sorry for the noise, I hadn't understood that the functionality I was looking for is already available via gi: I can create a WallClock and connect to it for changes. Nice!

I don't think we want this in glib anymore, do we? Let's close this.