Re: [PATCH] MI: new timing command

Nick Roberts
2007-01-27T22:08:58Z

> Okay, I tried this now, and on a system which has sys/resource.h and
> getrusage, it works both with HAVE_SYS_RESOURCE_H and HAVE_GETRUSAGE
> and without them (I hacked config.h to get it compiled both ways).

Hopefully you hacked config.h just to save time as my change to configure.ac
should handle that.

> However, I don't see how it can work on systems without sys/resource.h
> and without getrusage: your patch uses struct rusage unconditionally:
>
> mi-parse.h:
>
> + #include <sys/resource.h>
> +
> + /* Timestamps for current command and last asynchronous command */
> + struct mi_timestamp {
> + struct timeval wallclock;
> + struct rusage rusage;
> + };
> +
>
> m-main.c:
>
> + static void
> + timestamp (struct mi_timestamp *tv)
> + {
> + long usec;
> + #ifdef HAVE_GETRUSAGE
> + gettimeofday (&tv->wallclock, NULL);
> + getrusage (RUSAGE_SELF, &tv->rusage);
> + #else
> + usec = get_run_time ();
> + tv->wallclock.tv_sec = usec/1000000;
> + tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
> + tv->rusage.ru_utime.tv_sec = 0;
> + tv->rusage.ru_utime.tv_usec = 0;
> + tv->rusage.ru_stime.tv_sec = 0;
> + tv->rusage.ru_stime.tv_usec = 0;
> + #endif
> + }
>
>
> Note that mi-parse.h includes sys/resource.h unconditionally, which
> will fail to compile on systems that don't have that header; and both
> mi-parse.h and mi-main.c use struct rusage.

Duh! I see that now.

> Also, in mi-main.c, there's no need to use gettimeofday only in
> the HAVE_GETRUSAGE branch, as its availability is independent of
> HAVE_GETRUSAGE.

Well it was explained to me that get_run_time basically returns wallclock
time when getrusage isn't defined. If that is the case, I could just
gettimeofday for both cases and never use get_run_time instead.

> I suggest to use gettimeofday to compute wallclock time on all
> platforms, and avoid using struct rusage in struct mi_timestamp,
> e.g. like this:
>
> struct mi_timestamp {
> struct timeval wallclock;
> struct timeval utime;
> struct timeval stime'
> }
>
> Then in mi-main.c:timestamp you could copy the values from what
> getrusage returns to the utime and stime members of mi_timestamp, when
> getrusage is available, and if not, assign the value returned by
> get_run_time to members of utime.

Won't the members wallclock and utime be basically the same now?

> If you submit a modified patch that does the above, I will test it on
> a platform that doesn't have getrusage.

Would something like below work? When getrusage isn't defined I just use
wallclock. There's no need to define utime or stime in this case as the
values are always 0.

--
Nick http://www.inet.net.nz/~nickrob

*** mi-parse.h 28 Jan 2007 10:54:38 +1300 1.6
--- mi-parse.h 28 Jan 2007 10:39:02 +1300
***************
*** 24,29 ****
--- 24,41 ----

/* MI parser */

+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+
+ /* Timestamps for current command and last asynchronous command. */
+ struct mi_timestamp {
+ struct timeval wallclock;
+ #ifdef HAVE_GETRUSAGE
+ struct rusage rusage;
+ #endif
+ };
+
enum mi_command_type
{
MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 47,53 ----
char *command;
char *token;
const struct mi_cmd *cmd;
+ struct mi_timestamp *cmd_start;
char *args;
char **argv;
int argc;

*** mi-main.c 23 Jan 2007 20:21:56 +1300 1.91
--- mi-main.c 28 Jan 2007 10:44:28 +1300
*************** _initialize_mi_main (void)
*** 1457,1459 ****
--- 1513,1562 ----
DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
}
+
+ static void
+ timestamp (struct mi_timestamp *tv)
+ {
+ long usec;
+ #ifdef HAVE_GETRUSAGE
+ gettimeofday (&tv->wallclock, NULL);
+ getrusage (RUSAGE_SELF, &tv->rusage);
+ #else
+ usec = get_run_time ();
+ tv->wallclock.tv_sec = usec/1000000;
+ tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
+ #endif
+ }
+
+ static void
+ print_diff_now (struct mi_timestamp *start)
+ {
+ struct mi_timestamp now;
+ timestamp (&now);
+ print_diff (start, &now);
+ }
+
+ static long
+ timeval_diff (struct timeval start, struct timeval end)
+ {
+ return ((end.tv_sec - start.tv_sec) * 1000000)
+ + (end.tv_usec - start.tv_usec);
+ }
+
+ static void
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+ {
+ fprintf_unfiltered
+ (raw_stdout,
+ ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}",
+ timeval_diff (start->wallclock, end->wallclock) / 1000000.0,
+ #ifdef HAVE_GETRUSAGE
+ timeval_diff (start->rusage.ru_utime, end->rusage.ru_utime)
+ / 1000000.0,
+ timeval_diff (start->rusage.ru_stime, end->rusage.ru_stime)
+ / 1000000.0
+ #else
+ 0, 0
+ #endif
+ );
+ }