diff mbox

[libfortran] PR 47571 Some janitorial cleanup

Message ID AANLkTim=6rBKGE248cQpWt2+ZqZC+VBHJehFvrU9dV8h@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist Feb. 2, 2011, 7:23 p.m. UTC 
Hi,

I reviewed the Gfortran time intrinsics, both standard and extensions,
and AFAIK the only place where a monotonic clock fits the API, and
where we can expose high resolution, is the SYSTEM_CLOCK intrinsic.

Now, due to the PR 47571 fix where we now call clock_gettime() via a
weakref, some of the original reason behind the gf_gettime() wrapper
has been lost, as now we still need to include fallback code in case
the weakref is not available. This needlessly bloats the code for
date_and_time() which is unable to use either the extra precision and
needs a realtime clock, not a monotonic clock. Additionally, in some
not quite bleeding edge Linux kernels, gettimeofday() is a vsyscall
and clock_gettime() isn't, leading to higher overhead for no benefit
(the difference is something like 1e6 vs 1e7 calls/s on current hw, so
it's exceedingly unlikely to actually matter in reality).  The only
fly in the ointment here is that POSIX 2008 has marked gettimeofday()
as obsolescent, however I don't see this mattering in reality for the
foreseeable future.

The attached patch moves all the weakref trickery and clock_gettime()
usage from time_1.h to system_clock.c, where the only benefactor also
lives.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

2011-02-02  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/47571
	* intrinsics/time_1.h (GF_CLOCK_MONOTONIC): Move to system_clock.c.
	(weak_gettime): Likewise.
	(gf_gettime): Change API, move weak_gettime() usage to
	system_clock.c
	* intrinsics/system_clock.c (gf_gettime_mono): New function.
	(system_clock_4): Use gf_gettime_mono().
	(system_clock_8): Likewise.
	* intrinsics/date_and_time.c (date_and_time): Update gf_gettime()
	usage.

Comments

Steve Kargl Feb. 2, 2011, 7:41 p.m. UTC | #1 
On Wed, Feb 02, 2011 at 09:23:15PM +0200, Janne Blomqvist wrote:
> 
> I reviewed the Gfortran time intrinsics, both standard and extensions,
> and AFAIK the only place where a monotonic clock fits the API, and
> where we can expose high resolution, is the SYSTEM_CLOCK intrinsic.
> 

I suppose that there is some subtle reason that I do not
see.  But, why should cpu_time() lack high resolution
if it is available?
Janne Blomqvist Feb. 3, 2011, 9:34 a.m. UTC | #2 
On Wed, Feb 2, 2011 at 21:41, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Wed, Feb 02, 2011 at 09:23:15PM +0200, Janne Blomqvist wrote:
>>
>> I reviewed the Gfortran time intrinsics, both standard and extensions,
>> and AFAIK the only place where a monotonic clock fits the API, and
>> where we can expose high resolution, is the SYSTEM_CLOCK intrinsic.
>>
>
> I suppose that there is some subtle reason that I do not
> see.  But, why should cpu_time() lack high resolution
> if it is available?

Sorry, I meant the various wallclock time intrinsics. And with "high
resolution", I was thinking of clock_gettime() (ns resolution) vs.
gettimeofday() (us resolution).

cpu_time() does indeed allow high resolution process CPU usage timing,
on POSIX targets we use getrusage() (if available) which allows us
resolution. Although at least on Linux the practical resolution is at
the granularity of the timer interrupt which depending on kernel
compile options is somewhere between 1 and 10 ms.
diff mbox

Patch

diff --git a/libgfortran/intrinsics/date_and_time.c b/libgfortran/intrinsics/date_and_time.c
index 714df14..c58d114 100644
--- a/libgfortran/intrinsics/date_and_time.c
+++ b/libgfortran/intrinsics/date_and_time.c
@@ -162,11 +162,11 @@  date_and_time (char *__date, char *__time, char *__zone,
   struct tm local_time;
   struct tm UTC_time;
 
-  long nanosecs;
+  long usecs;
 
-  if (!gf_gettime(GF_CLOCK_REALTIME, &lt, &nanosecs))
+  if (!gf_gettime (&lt, &usecs))
     {
-      values[7] = nanosecs / 1000000;
+      values[7] = usecs / 1000;
 
       localtime_r (&lt, &local_time);
       gmtime_r (&lt, &UTC_time);
diff --git a/libgfortran/intrinsics/system_clock.c b/libgfortran/intrinsics/system_clock.c
index 3715562..17d3949 100644
--- a/libgfortran/intrinsics/system_clock.c
+++ b/libgfortran/intrinsics/system_clock.c
@@ -29,6 +29,67 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include "time_1.h"
 
+#ifdef HAVE_CLOCK_GETTIME
+/* POSIX states that CLOCK_REALTIME must be present if clock_gettime
+   is available, others are optional.  */
+#ifdef CLOCK_MONOTONIC
+#define GF_CLOCK_MONOTONIC CLOCK_MONOTONIC
+#else
+#define GF_CLOCK_MONOTONIC CLOCK_REALTIME
+#endif
+
+/* Weakref trickery for clock_gettime().  On Glibc, clock_gettime()
+   requires us to link in librt, which also pulls in libpthread.  In
+   order to avoid this by default, only call clock_gettime() through a
+   weak reference.  */
+#ifdef SUPPORTS_WEAK
+static int weak_gettime (clockid_t, struct timespec *) 
+  __attribute__((__weakref__("clock_gettime")));
+#else
+static inline int weak_gettime (clockid_t clk_id, struct timespec *res)
+{
+  return clock_gettime (clk_id, res);
+}
+#endif
+#endif
+
+
+/* High resolution monotonic clock, falling back to the realtime clock
+   if the target does not support such a clock.
+
+   Arguments:
+   secs     - OUTPUT, seconds
+   nanosecs - OUTPUT, nanoseconds
+
+   If the target supports a monotonic clock, the OUTPUT arguments
+   represent a monotonically incrementing clock starting from some
+   unspecified time in the past.
+
+   If a monotonic clock is not available, falls back to the realtime
+   clock which is not monotonic.
+
+   Return value: 0 for success, -1 for error. In case of error, errno
+   is set.
+*/
+static inline int
+gf_gettime_mono (time_t * secs, long * nanosecs)
+{
+  int err;
+#ifdef HAVE_CLOCK_GETTIME
+  if (weak_gettime)
+    {
+      struct timespec ts;
+      err = weak_gettime (GF_CLOCK_MONOTONIC, &ts);
+      *secs = ts.tv_sec;
+      *nanosecs = ts.tv_nsec;
+      return err;
+    }
+#endif
+  err = gf_gettime (secs, nanosecs);
+  *nanosecs *= 1000;
+  return err;
+}
+
 extern void system_clock_4 (GFC_INTEGER_4 *, GFC_INTEGER_4 *, GFC_INTEGER_4 *);
 export_proto(system_clock_4);
 
@@ -56,7 +117,7 @@  system_clock_4(GFC_INTEGER_4 *count, GFC_INTEGER_4 *count_rate,
   if (sizeof (secs) < sizeof (GFC_INTEGER_4))
     internal_error (NULL, "secs too small");
 
-  if (gf_gettime (GF_CLOCK_MONOTONIC, &secs, &nanosecs) == 0)
+  if (gf_gettime_mono (&secs, &nanosecs) == 0)
     {
       GFC_UINTEGER_4 ucnt = (GFC_UINTEGER_4) secs * TCK;
       ucnt += (nanosecs + 500000000 / TCK) / (1000000000 / TCK);
@@ -103,7 +164,7 @@  system_clock_8 (GFC_INTEGER_8 *count, GFC_INTEGER_8 *count_rate,
   if (sizeof (secs) < sizeof (GFC_INTEGER_4))
     internal_error (NULL, "secs too small");
 
-  if (gf_gettime (GF_CLOCK_MONOTONIC, &secs, &nanosecs) == 0)
+  if (gf_gettime_mono (&secs, &nanosecs) == 0)
     {
       GFC_UINTEGER_8 ucnt = (GFC_UINTEGER_8) secs * TCK;
       ucnt += (nanosecs + 500000000 / TCK) / (1000000000 / TCK);
diff --git a/libgfortran/intrinsics/time_1.h b/libgfortran/intrinsics/time_1.h
index 86e4331..073595a 100644
--- a/libgfortran/intrinsics/time_1.h
+++ b/libgfortran/intrinsics/time_1.h
@@ -175,87 +175,40 @@  gf_cputime (long *user_sec, long *user_usec, long *system_sec, long *system_usec
 #endif
 
 
-/* POSIX states that CLOCK_REALTIME must be present if clock_gettime
-   is available, others are optional.  */
-#ifdef CLOCK_REALTIME
-#define GF_CLOCK_REALTIME CLOCK_REALTIME
-#else
-#define GF_CLOCK_REALTIME 0
-#endif
-
-#ifdef CLOCK_MONOTONIC
-#define GF_CLOCK_MONOTONIC CLOCK_MONOTONIC
-#else
-#define GF_CLOCK_MONOTONIC GF_CLOCK_REALTIME
-#endif
-
-/* Weakref trickery for clock_gettime().  On Glibc, clock_gettime()
-   requires us to link in librt, which also pulls in libpthread.  In
-   order to avoid this by default, only call clock_gettime() through a
-   weak reference.  */
-#ifdef HAVE_CLOCK_GETTIME
-#ifdef SUPPORTS_WEAK
-static int weak_gettime (clockid_t, struct timespec *) 
-  __attribute__((__weakref__("clock_gettime")));
-#else
-static inline int weak_gettime (clockid_t clk_id, struct timespec *res)
-{
-  return clock_gettime (clk_id, res);
-}
-#endif
-#endif
+/* Realtime clock with microsecond resolution, falling back to less
+   precise functions if the target does not support gettimeofday().
 
-/* Arguments:
-   clock_id - INPUT, must be either GF_CLOCK_REALTIME or GF_CLOCK_MONOTONIC
+   Arguments:
    secs     - OUTPUT, seconds
-   nanosecs - OUTPUT, OPTIONAL, nanoseconds
+   usecs    - OUTPUT, microseconds
 
-   If clock_id equals GF_CLOCK_REALTIME, the OUTPUT arguments shall be
-   the number of seconds and nanoseconds since the Epoch. If clock_id
-   equals GF_CLOCK_MONOTONIC, and if the target supports it, the
-   OUTPUT arguments represent a monotonically incrementing clock
-   starting from some unspecified time in the past.
+   The OUTPUT arguments shall represent the number of seconds and
+   nanoseconds since the Epoch.
 
    Return value: 0 for success, -1 for error. In case of error, errno
    is set.
 */
 static inline int
-gf_gettime (int clock_id __attribute__((unused)), time_t * secs, 
-            long * nanosecs)
+gf_gettime (time_t * secs, long * usecs)
 {
-#ifdef HAVE_CLOCK_GETTIME
-  if (weak_gettime)
-    {
-      struct timespec ts;
-      int err;
-      err = weak_gettime (clock_id, &ts);
-      *secs = ts.tv_sec;
-      if (nanosecs)
-	*nanosecs = ts.tv_nsec;
-      return err;
-    }
-#endif
 #ifdef HAVE_GETTIMEOFDAY
   struct timeval tv;
   int err;
   err = gettimeofday (&tv, NULL);
   *secs = tv.tv_sec;
-  if (nanosecs)
-    *nanosecs = tv.tv_usec * 1000;
+  *usecs = tv.tv_usec;
   return err;
 #elif HAVE_TIME
   time_t t, t2;
   t = time (&t2);
   *secs = t2;
-  if (nanosecs)
-    *nanosecs = 0;
+  *usecs = 0;
   if (t == ((time_t)-1))
     return -1;
   return 0;
 #else
   *secs = 0;
-  if (nanosecs)
-    *nanosecs = 0;
+  *usecs = 0;
   errno = ENOSYS;
   return -1;
 #endif