Patchwork [libfortran] PR 47571 Some janitorial cleanup

login
register
mail settings
Submitter Janne Blomqvist
Date Feb. 3, 2011, 12:45 p.m.
Message ID <AANLkTikWVbdF113pgV-RvUb2yxLdmJt=Us-cZO_X608a@mail.gmail.com>
Download mbox | patch
Permalink /patch/81638/
State New
Headers show

Comments

Janne Blomqvist - Feb. 3, 2011, 12:45 p.m.
On Wed, Feb 2, 2011 at 21:23, Janne Blomqvist <blomqvist.janne@gmail.com> wrote:
> 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.

Hi,

updated patch attached. Compared to the previous one this might fix
the build regression reported on HP-UX which does support weak symbols
but apparently does barf on weak undefined references. This is done in
a slightly kludgy way, in that in acinclude.m4 we previously have set
GTHREAD_USE_WEAK to 0 for such targets, hence in the updated patch
GTHREAD_USE_WEAK is defined and set to 1 if it's not previously
present, and then we check for both SUPPORTS_WEAK and
GTHREAD_USE_WEAK.

2011-02-03  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 (GTHREAD_USE_WEAK): Define.
	(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.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
Jerry DeLisle - Feb. 5, 2011, 3:58 p.m.
On 02/03/2011 04:45 AM, Janne Blomqvist wrote:
> On Wed, Feb 2, 2011 at 21:23, Janne Blomqvist<blomqvist.janne@gmail.com>  wrote:
>> 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.
>
> Hi,
>
> updated patch attached. Compared to the previous one this might fix
> the build regression reported on HP-UX which does support weak symbols
> but apparently does barf on weak undefined references. This is done in
> a slightly kludgy way, in that in acinclude.m4 we previously have set
> GTHREAD_USE_WEAK to 0 for such targets, hence in the updated patch
> GTHREAD_USE_WEAK is defined and set to 1 if it's not previously
> present, and then we check for both SUPPORTS_WEAK and
> GTHREAD_USE_WEAK.
>
> 2011-02-03  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 (GTHREAD_USE_WEAK): Define.
> 	(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.
>
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>

OK and thanks for patch.

Jerry
Janne Blomqvist - Feb. 5, 2011, 4:22 p.m.
On Sat, Feb 5, 2011 at 17:58, Jerry DeLisle <jvdelisle@frontier.com> wrote:
> On 02/03/2011 04:45 AM, Janne Blomqvist wrote:
>>
>> On Wed, Feb 2, 2011 at 21:23, Janne Blomqvist<blomqvist.janne@gmail.com>
>>  wrote:
>>>
>>> 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.
>>
>> Hi,
>>
>> updated patch attached. Compared to the previous one this might fix
>> the build regression reported on HP-UX which does support weak symbols
>> but apparently does barf on weak undefined references. This is done in
>> a slightly kludgy way, in that in acinclude.m4 we previously have set
>> GTHREAD_USE_WEAK to 0 for such targets, hence in the updated patch
>> GTHREAD_USE_WEAK is defined and set to 1 if it's not previously
>> present, and then we check for both SUPPORTS_WEAK and
>> GTHREAD_USE_WEAK.
>>
>> 2011-02-03  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 (GTHREAD_USE_WEAK): Define.
>>        (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.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK and thanks for patch.
>
> Jerry
>

Thanks,

Sending        libgfortran/ChangeLog
Sending        libgfortran/intrinsics/date_and_time.c
Sending        libgfortran/intrinsics/system_clock.c
Sending        libgfortran/intrinsics/time_1.h
Transmitting file data ....
Committed revision 169852.

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..3a44dd9 100644
--- a/libgfortran/intrinsics/system_clock.c
+++ b/libgfortran/intrinsics/system_clock.c
@@ -29,6 +29,75 @@  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. 
+
+   Some targets don't support weak undefined references; on these
+   GTHREAD_USE_WEAK is 0. So we need to define it to 1 on other
+   targets.  */
+#ifndef GTHREAD_USE_WEAK
+#define GTHREAD_USE_WEAK 1
+#endif
+
+#if SUPPORTS_WEAK && GTHREAD_USE_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 +125,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 +172,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