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: <4509E189.9020504@roguewave.com>
Date: Thu, 14 Sep 2006 17:11:05 -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> <45097C36.3050603@roguewave.com>
 <4509D685.6090808@roguewave.com>
In-Reply-To: <4509D685.6090808@roguewave.com>
Content-Type: multipart/mixed;
 boundary="------------000209060406060605090901"

--------------000209060406060605090901
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

How does a tweaked patch (and changelog) sound instead of a follow up patch?

--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.
       print_header_plain: Alter cols for process times.
       print_target_plain: Partition output column printing by section, 
add timing output.
     * exec.h (exec_file): Alter signature to accept target_status 
rather than char**.
     * exec.cpp (display.h): Include.
       (calculate_usage) [!_WIN32 && !_WIN64]: Define function to 
populate user and sys fields of provided target_status struct (if 
_XOPEN_UNIX is defined).
       (exec_file): Alter to accept target_status rather than char**, 
use argv element in place of old char** argument.
       (exec_file) [!_WIN32 && !_WIN64]: call calculate_usage after 
wait_for_child.
     * runall.cpp (run_target): Pass target_status struct rather than 
argv element.


Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings all
>>
>> Attached is the revised version of this patch, now that the display 
>> subsystem has been implemented.
> 
> This looks pretty good. One comment: it's good to avoid preprocessor
> conditionals as much as possible so that the same code gets compiled
> regardless of the platform (smaller chance of breakage that way).
> (Note that in library development this rule is secondary to space
> and speed efficiency.)
> 
> So...
> 
> [...]
>> Index: exec.cpp
>> ===================================================================
>> --- exec.cpp    (revision 443137)
>> +++ exec.cpp    (working copy)
> [...]
>> @@ -690,6 +691,60 @@
>>          }
>>      }
>>  }
> [...]
>> +static void
>> +calculate_usage (struct target_status* result)
> 
> ...this function should be defined and called regardless of whether
> or not it knows how to compute the usage (it could be empty when it
> doesn't), and its caller(s)...
> 
> [...]
>> @@ -708,15 +763,17 @@
>>     @see wait_for_child ()
>>  */
>>  struct exec_attrs -exec_file (char** argv)
>> +exec_file (struct target_status* result)
> [...]
>> @@ -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 */
> 
> ...should call it unconditionally and should be prepared for it
> not to be able to produce meaningful values.
> 
> [...]
>> 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
> 
> Similarly this function, instead of using the preprocessor, should
> take an argument telling it which of the two headers to print (or
> it could print the first one unconditionally), and...
> 
>> @@ -48,19 +60,31 @@
> [...]
>> +    /* 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
> 
> ...this call to printf should be guarded by a runtime conditional
> rather than a preprocessor one.
> 
> I'm fine committing the code as is as long as this patch is followed
> by one implementing the suggested changes sometime (very) soon  :)
> Let me know which way you want to proceed.
> 
> Thanks
> Martin

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

Index: display.h
===================================================================
--- display.h	(revision 443496)
+++ 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 443496)
+++ 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"
@@ -697,6 +698,62 @@
 #endif /* _XOPEN_UNIX */
 
 /**
+   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)
+{
+#ifdef _XOPEN_UNIX
+    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 */
+}
+
+/**
    Entry point to the child process (watchdog) subsystem.
 
    This method fork ()s, creating a child process.  This child process becomes
@@ -712,15 +769,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)*/
@@ -752,7 +811,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, 
@@ -776,10 +835,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);
     }
@@ -787,13 +846,15 @@
     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);
+        calculate_usage (result);
+        return state;
     }
-    
-    /* parent */
-    return wait_for_child (child_pid);
 }
 #else  /* _WIN{32,64} */
 /**
@@ -950,7 +1011,7 @@
    @see wait_for_child ()
 */
 struct exec_attrs 
-exec_file (char** argv)
+exec_file (struct target_status* result)
 {
     char* merged;
     PROCESS_INFORMATION child;
@@ -962,7 +1023,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);
 
@@ -979,7 +1042,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, 
@@ -1003,7 +1066,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
@@ -1011,7 +1074,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 443496)
+++ display.cpp	(working copy)
@@ -26,6 +26,9 @@
 
 #include <assert.h>
 #include <stdio.h>      /* for fflush(), printf(), puts(), ... */
+#if !defined (_WIN32) && !defined (_WIN64)
+#  include <unistd.h> /* for _XOPEN_UNIX */
+#endif
 
 #include "cmdopt.h" /* for target_name -should this be moved? */
 #include "exec.h" /* for get_signame */
@@ -34,7 +37,8 @@
 
 void print_header_plain ()
 {
-    puts ("NAME                      STATUS ASSERTS FAILED PERCNT");
+    puts ("NAME                      STATUS    USER     SYS ASSERTS FAILED "
+          "PERCNT");
 }
 
 void print_target_plain (const struct target_status*)
@@ -48,19 +52,29 @@
     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 */
+    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 ("                ");
+
+    /* 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 443496)
+++ 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 443496)
+++ 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);
     }
 

--------------000209060406060605090901--