You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Andrew Black <ab...@roguewave.com> on 2006/08/24 00:03:31 UTC
[patch] Child process stats in exec utility (unix)
Greetings all
Attached is a partial patch that allows statistics to be gathered on
child processes. At this time, the only statistics gathered are user
time and CPU time, though it should be possible to gather others.
However, without a way to display these stats to the user, collecting
them has little value. Unfortunately, adding this information to the
information that is displayed in the output is more difficult than it
might first appear. This difficulty is due to the fragmented nature of
the output. The table header is produced in one place (main()), the row
headers in a second (run_target()), with the remainder of the status
information scattered across 5 different places (check_target_ok() for
build/file system failure, process_results() for non-0 child process
termination, and check_test()/check_compat_test()/check_example() for
output analysis).
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.
--Andrew Black
Re: [patch] Child process stats in exec utility (unix)
Posted by Martin Sebor <se...@roguewave.com>.
Also, I should have mentioned: Please use [PATCH] in all caps
when posting patches as I described before.
Thanks
Martin
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
>
Re: [patch] Child process stats in exec utility (unix)
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> I have found myself unable to replicate the bogus time issue you have
> observed.
>
> I have tried using multiple tests on my linux box, along with the
> 27.stringbuf.virtuals test on both Solaris 10 and Solaris 9. Perhaps
> automated testing will provide a replication of the issue.
I suspect the bogus output was actually my problem. After cleaning
out my directory and rebuilding the utility from scratch the numbers
look okay. I wonder if my dependencies are out of date or if there's
a bug in the dependency generation code. Sorry about sending you on
a wild goose chase...
>
> Attached is another revised patch that moves the columns as proposed.
> I'm not certain if the revised output looks as good for examples, but I
> have no preference either way.
Okay, it's in: http://svn.apache.org/viewvc?view=rev&rev=446689
Martin
PS In your ChangeLog (as well as in code), preprocessor expressions
like (_WIN32 || _WIN64) can be simplified to just _WIN32 since the
macro is guaranteed to be #defined whenever _WIN64 is #defined.
We should change existing code and simplify the expression.
Martin
Re: [patch] Child process stats in exec utility (unix)
Posted by Andrew Black <ab...@roguewave.com>.
I have found myself unable to replicate the bogus time issue you have
observed.
I have tried using multiple tests on my linux box, along with the
27.stringbuf.virtuals test on both Solaris 10 and Solaris 9. Perhaps
automated testing will provide a replication of the issue.
Attached is another revised patch that moves the columns as proposed.
I'm not certain if the revised output looks as good for examples, but I
have no preference either way.
--Andrew Black
Martin Sebor wrote:
> Andrew Black wrote:
>> How does a tweaked patch (and changelog) sound instead of a follow up
>> patch?
>
> It sounds good except for the bogus times. I think we should fix
> it before committing the changes. Also, wouldn't displaying the
> times as the last two columns work better? I'm kind of used to
> seeing the status and assertions together.
>
> Martin
Re: [patch] Child process stats in exec utility (unix)
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> How does a tweaked patch (and changelog) sound instead of a follow up
> patch?
It sounds good except for the bogus times. I think we should fix
it before committing the changes. Also, wouldn't displaying the
times as the last two columns work better? I'm kind of used to
seeing the status and assertions together.
Martin
Re: [patch] Child process stats in exec utility (unix)
Posted by Andrew Black <ab...@roguewave.com>.
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
Re: [patch] Child process stats in exec utility (unix)
Posted by Martin Sebor <se...@roguewave.com>.
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
Re: [patch] Child process stats in exec utility (unix)
Posted by Andrew Black <ab...@roguewave.com>.
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
Re: [patch] Child process stats in exec utility (unix)
Posted by Andrew Black <ab...@roguewave.com>.
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
Re: [patch] Child process stats in exec utility (unix)
Posted by Martin Sebor <se...@roguewave.com>.
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