Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: pass (hermes.apache.org: local policy)
Message-ID: <45097C36.3050603@roguewave.com>
Date: Thu, 14 Sep 2006 09:58:46 -0600
From: Andrew Black <ablack@roguewave.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.0.6) Gecko/20060730 SeaMonkey/1.0.4
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [patch] Child process stats in exec utility (unix)
References: <44ECD0B3.5010203@roguewave.com> <44EE07B5.5060204@roguewave.com>
 <44EE0AB2.6020002@roguewave.com>
In-Reply-To: <44EE0AB2.6020002@roguewave.com>
Content-Type: multipart/mixed;
 boundary="------------030406000204050108080803"

--------------030406000204050108080803
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Greetings all

Attached is the revised version of this patch, now that the display 
subsystem has been implemented.

--Andrew Black

Log:
	* display.h (unistd.h) [!_WIN32 && !_WIN64]: Include.
	  (sys/time.h) [_XOPEN_UNIX]: Include.
	  (rw_time_t, rw_suseconds_t, struct rw_timeval) [!_XOPEN_UNIX]: Define 
placeholder structures.
	  (rw_timeval): Define abstraction typedef.
	  (struct target_status): Add run, user, sys elements.
	* display.cpp (unistd.h) [!_WIN32 && !_WIN64]: Include.
	  CHILD_STATS [_XOPEN_UNIX]: Define convenience macro.
	  print_header_plain: Alter output if CHILD_STATS is defined.
	  print_target_plain: Partition output column printing by section, add 
timing output if CHILD_STATS is defined.
	* exec.h (exec_file): Alter signature to accept target_status rather 
than char**.
	* exec.cpp (display.h): Include.
	  (calculate_usage) [_XOPEN_UNIX]: Define function to populate user and 
sys fields of provided target_status struct.
	  (exec_file): Alter to accept target_status rather than char**,  use 
argv element in place of old char** argument.
	  (exec_file) [!_WIN32 && !_WIN64 && _XOPEN_UNIX]: call calculate_usage 
after wait_for_child.
	* runall.cpp (run_target): Pass target_status struct rather than  argv 
element.

Andrew Black wrote:
> Martin Sebor wrote:
>> Andrew Black wrote:
>> [...]
>>>
>>> It appears to me that a partial rewrite is necessary to split the 
>>> result display into its own subsystem.  This rewrite would accomplish 
>>> a couple things.  First, it should allow the addition of new columns 
>>> to the output in a simpler manner.  Second, it should allow for the 
>>> output to be formatted for for targets other than plain text 
>>> terminals.  Both of these changes would likely be considered 
>>> beneficial, but will require a separate patch.
>>
>> Yes, I also think that will be necessary. Should I assume that
>> you don't expect this patch to be committed and plan to post
>> a more complete solution in its place? (One that also deals
>> with the Windows side of things?)
>>
>> Martin
> 
> 
> I think that would be an accurate enough analysis of the situation.  My 
> (rough) plan is to create the display subsystem in one patch, then 
> return to this task once that patch has been completed and use that new 
> subsystem to display the additional statistics in a follow-up patch.
> 
> --Andrew Black

--------------030406000204050108080803
Content-Type: text/x-patch;
 name="childstats3.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="childstats3.diff"

Index: display.h
===================================================================
--- display.h	(revision 443137)
+++ display.h	(working copy)
@@ -27,7 +27,39 @@
 #ifndef RW_DISPLAY_H
 #define RW_DISPLAY_H
 
+#if !defined (_WIN32) && !defined (_WIN64)
+#  include <unistd.h> /* for _XOPEN_UNIX */
+#endif
+
+#ifdef _XOPEN_UNIX
+#  include <sys/time.h> /* for struct timeval */
 /**
+   Abstraction typedef for struct timeval using real struct
+*/
+typedef struct timeval rw_timeval;
+#else
+/**
+   Placeholder time_t for use in rw_timeval
+*/
+typedef long rw_time_t;
+/**
+   Placeholder suseconds_t for use in rw_timeval
+*/
+typedef long rw_suseconds_t;
+/**
+   Placeholder struct timeval to use if _XOPEN_UNIX isn't defined
+*/
+struct rw_timeval {
+    rw_time_t tv_sec;
+    rw_suseconds_t tv_usec;
+};
+/**
+   Abstraction typedef for struct timeval using placeholder struct
+*/
+typedef struct rw_timeval rw_timeval;
+#endif
+
+/**
    Output format mode enumeration.
 
    Used to determine the mode used in producing output.
@@ -74,7 +106,10 @@
     char** argv; /**< Target argv array. */
     int exit; /**< exit code for process. */
     int signal; /**< Termination signal for process. */
+    int run; /**< Flag indicating if the process executed. */
     enum ProcessStatus status; /**< Textual process status. */
+    rw_timeval user; /**< Elapsed user time spent in execution. */
+    rw_timeval sys; /**< Elapsed system time spent in execution. */
     unsigned warn; /**< Number of (test) warnings. */
     unsigned assert; /**< Number of (test) assertions. */
     unsigned failed; /**< Number of failed (test) assertions. */
Index: exec.cpp
===================================================================
--- exec.cpp	(revision 443137)
+++ exec.cpp	(working copy)
@@ -51,6 +51,7 @@
 #include <sys/types.h>
 
 #include "cmdopt.h"
+#include "display.h" /* for struct target_status */
 #include "util.h"
 
 #include "exec.h"
@@ -690,6 +691,60 @@
         }
     }
 }
+
+/**
+   Calculates the amount of resources used by the child processes.
+
+   This method uses the getrusage() system call to calculate the resources
+   used by the child process.  However, getrusage() only is able to calcualte
+   agragate usage by all child processes, not usage by a specific child process.
+   Therefore, we must keep a running tally of how many resources had been used
+   the previous time we calculated the usage.  This difference is the resources
+   that were used by the process that just completed.
+
+   @param result target_status structure to populate with process usage.
+*/
+static void
+calculate_usage (struct target_status* result)
+{
+    static bool init = 0;
+    static struct rusage history;
+    struct rusage now;
+
+    assert (0 != result);
+
+    if (!init) {
+        memset (&history, 0, sizeof history);
+        init = 1;
+    }
+
+    if (0 != getrusage (RUSAGE_CHILDREN, &now)) {
+        warn ("Failed to retrieve child resource usage: %s", strerror (errno));
+        return;
+    }
+
+    /* time calculations */
+    result->user.tv_sec = now.ru_utime.tv_sec - history.ru_utime.tv_sec;
+    result->user.tv_usec = now.ru_utime.tv_usec - history.ru_utime.tv_usec;
+    result->sys.tv_sec = now.ru_stime.tv_sec - history.ru_stime.tv_sec;
+    result->sys.tv_usec = now.ru_stime.tv_usec - history.ru_stime.tv_usec;
+
+    /* Adjust seconds/microseconds */
+    if (now.ru_utime.tv_usec < history.ru_utime.tv_usec) {
+        --result->user.tv_sec;
+        result->user.tv_usec += 1000000;
+    }
+    if (now.ru_stime.tv_usec < history.ru_stime.tv_usec) {
+        --result->sys.tv_sec;
+        result->sys.tv_usec += 1000000;
+    }
+
+    /* Tag result as having run */
+    result->run = 1;
+
+    /* Cache the values retrieved */
+    memcpy (&history, &now, sizeof (struct rusage));
+}
 #endif /* _XOPEN_UNIX */
 
 /**
@@ -708,15 +763,17 @@
    @see wait_for_child ()
 */
 struct exec_attrs 
-exec_file (char** argv)
+exec_file (struct target_status* result)
 {
     const pid_t child_pid = fork ();
 
+    assert (0 != result);
+    assert (0 != result->argv);
+    assert (0 != result->argv [0]);
+
     if (0 == child_pid) {   /* child */
         FILE* error_file;
 
-        assert (0 != argv);
-        assert (0 != argv [0]);
         assert (0 != target_name);
 
         /* Set process group ID (so entire group can be killed)*/
@@ -748,7 +805,7 @@
 
         /* Redirect stdout */
         {
-            char* const tmp_name = output_name (argv [0]);
+            char* const tmp_name = output_name (result->argv [0]);
             int intermit;
 
             intermit = open (tmp_name, O_WRONLY | O_CREAT | O_TRUNC, 
@@ -772,10 +829,10 @@
         limit_process ();
 #endif /* _XOPEN_UNIX */
 
-        execv (argv [0], argv);
+        execv (result->argv [0], result->argv);
 
         fprintf (error_file, "%s (%s): execv (\"%s\", ...) error: %s\n",
-                 exe_name, target_name, argv [0], strerror (errno));
+                 exe_name, target_name, result->argv [0], strerror (errno));
 
         exit (1);
     }
@@ -783,13 +840,17 @@
     if (-1 == child_pid) {
         /* Fake a failue to execute status return structure */
         struct exec_attrs state = {127, -1};
-        warn ("Unable to create child process for %s: %s\n", argv [0],
+        warn ("Unable to create child process for %s: %s\n", result->argv [0],
               strerror (errno));
         return state;
+    } else {
+        /* parent */
+        struct exec_attrs state = wait_for_child (child_pid);
+#ifdef _XOPEN_UNIX
+        calculate_usage (result);
+#endif /* _XOPEN_UNIX */
+        return state;
     }
-    
-    /* parent */
-    return wait_for_child (child_pid);
 }
 #else  /* _WIN{32,64} */
 /**
@@ -946,7 +1007,7 @@
    @see wait_for_child ()
 */
 struct exec_attrs 
-exec_file (char** argv)
+exec_file (struct target_status* result)
 {
     char* merged;
     PROCESS_INFORMATION child;
@@ -958,7 +1019,9 @@
     const DWORD real_timeout = (timeout > 0) ? timeout * 1000 : INFINITE;
     DWORD wait_code;
 
-    assert (0 != argv);
+    assert (0 != result);
+    assert (0 != result->argv);
+    assert (0 != result->argv [0]);
 
     memset (&status, 0, sizeof status);
 
@@ -975,7 +1038,7 @@
     /* Create I/O handles */
     {
         /* Output redirection */
-        char* const tmp_name = output_name (argv [0]);
+        char* const tmp_name = output_name (result->argv [0]);
 
         context.hStdOutput = CreateFile (tmp_name, GENERIC_WRITE, 
                 FILE_SHARE_WRITE, &child_sa, CREATE_ALWAYS, 
@@ -999,7 +1062,7 @@
         }
     }    
 
-    merged = merge_argv (argv);
+    merged = merge_argv (result->argv);
 
     /* set appropriate error mode (the child process inherits this
        error mode) to disable displaying the critical-error-handler
@@ -1007,7 +1070,7 @@
     UINT old_mode = SetErrorMode (SEM_FAILCRITICALERRORS
                                 | SEM_NOGPFAULTERRORBOX);
     /* Create the child process */
-    if (0 == CreateProcess (argv [0], merged, 0, 0, 1, 
+    if (0 == CreateProcess (result->argv [0], merged, 0, 0, 1, 
         CREATE_NEW_PROCESS_GROUP, 0, 0, &context, &child)) {
         /* record the status if we failed to create the process */
         status.status = -1;
Index: display.cpp
===================================================================
--- display.cpp	(revision 443137)
+++ display.cpp	(working copy)
@@ -26,7 +26,14 @@
 
 #include <assert.h>
 #include <stdio.h>      /* for fflush(), printf(), puts(), ... */
+#if !defined (_WIN32) && !defined (_WIN64)
+#  include <unistd.h> /* for _XOPEN_UNIX */
+#endif
 
+#ifdef _XOPEN_UNIX
+#  define CHILD_STATS
+#endif
+
 #include "cmdopt.h" /* for target_name -should this be moved? */
 #include "exec.h" /* for get_signame */
 
@@ -34,7 +41,12 @@
 
 void print_header_plain ()
 {
+#ifdef CHILD_STATS
+    puts ("NAME                      STATUS    USER     SYS ASSERTS FAILED "
+          "PERCNT");
+#else
     puts ("NAME                      STATUS ASSERTS FAILED PERCNT");
+#endif
 }
 
 void print_target_plain (const struct target_status*)
@@ -48,19 +60,31 @@
     assert (ST_OK <= status->status && ST_LAST > status->status);
 
     if (status->status) /* if status is set, print it */
-        printf ("%6s\n", short_st_name [status->status]);
+        printf ("%6s", short_st_name [status->status]);
     else if (status->exit) /* if exit code is non-zero, print it */
-        printf ("%6d\n", status->exit);
+        printf ("%6d", status->exit);
     else if (status->signal) /* if exit signal is non-zero, print it */
-        printf ("%6s\n", get_signame (status->signal));
-    else if (!status->assert) /* if the assertion count is 0, it's an example */
-        puts ("     0");
+        printf ("%6s", get_signame (status->signal));
+    else 
+        printf ("     0");
+
+    /* Print timings, if available */
+#ifdef CHILD_STATS
+    if (status->run && ST_NOT_KILLED != status->status)
+        printf (" %3ld.%03ld %3ld.%03ld", status->user.tv_sec,
+                status->user.tv_usec/1000, status->sys.tv_sec,
+                status->sys.tv_usec/1000);
+    else if (status->assert)
+        printf ("                ");
+#endif
+
+    /* Print assetions, if any registered */
+    if (status->assert) {
+        unsigned pcnt = 100 * (status->assert - status->failed) / status->assert;
+        printf (" %7u %6u %5d%%\n", status->assert, status->failed, pcnt);
+    }
     else {
-        unsigned pcnt = 0;
-        if (status->assert) {
-            pcnt = 100 * (status->assert - status->failed) / status->assert;
-        }
-        printf ("     0 %7u %6u %5d%%\n", status->assert, status->failed, pcnt);
+        puts ("");
     }
 }
 
Index: exec.h
===================================================================
--- exec.h	(revision 443137)
+++ exec.h	(working copy)
@@ -42,6 +42,6 @@
 
 const char* get_signame (int signo);
 
-struct exec_attrs exec_file (char** argv);
+struct exec_attrs exec_file (struct target_status* result);
 
 #endif   // RW_EXEC_H
Index: runall.cpp
===================================================================
--- runall.cpp	(revision 443137)
+++ runall.cpp	(working copy)
@@ -415,7 +415,7 @@
     print_target (&results);
 
     if (check_target_ok (&results)) {
-        struct exec_attrs status = exec_file (results.argv);
+        struct exec_attrs status = exec_file (&results);
         process_results (&status, &results);
     }
 

--------------030406000204050108080803--