Re: parallel autotest [1/3]: Refactor testsuite driver loop.

[Ralf Wildenhues]

Hi Eric,

* Eric Blake wrote on Thu, May 29, 2008 at 06:05:12PM CEST:
> Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes:
> > 
> >     * lib/autotest/general.m4 (AS_MESSAGE_LOG_FD): Move definition
> >     earlier in the file.

> This patch is worth applying, once these nits are addressed...

> > +# The directory containing helper files per test group.
> > +at_helper_dir=$at_suite_dir/at-groups
> 
> You know, at this point, an ascii tree diagram might be a helpful comment 
> (probably with # and not dnl, so that the comment is also present in the 
> generated testsuite).  Something like:
*snip*

Good point, I'll add something like that.

> > +# Stop file: if it exists, do not start new jobs.
> > +at_stop_file=$at_suite_dir/at-stop
> 
> Looks like a good idea.  Please mention it in the ChangeLog entry.

No problem.  I didn't mean to add it as public interface though,
not sure if that's what you intend it to be.

> > -}' "$at_myself" > "$at_test_source" &&
> > -. "$at_test_source" ||
> > +}' "$at_myself" > "$at_suite_dir/at-source-lines" &&
> > +. "$at_suite_dir/at-source-lines" ||

> I like the rename from $at_test_source to the literal at-source-lines, 
> especially since the comment on the former no longer matched reality after we 
> retooled things to use it as the line-number cache.

Yep.

> > +# at_func_group_prepare
> > +# ---------------------
> > +# Prepare running a test group
> > +at_func_group_prepare ()
> > +{
> > +  # The directory for additional per-group helper files.
> > +  at_job_dir=$at_helper_dir/$at_group
> > +  # The file containing the location of the last AT_CHECK.
> > +  at_check_line_file=$at_job_dir/at-check-line
> 
> Do we still need the at- prefix on files that live under $at_helper_dir?

No, I think we don't.

> It 
> seems a bit inconsistent to have $at_job_dir/at-stdout vs. $at_job_dir/pass, 
> but I'm not sure whether the latter should be at-pass or whether we can 
> simplify the former to stdout.

I think we should be able to simplify.  However, we can do that in a
separate patch, I think.  Or right away, I don't mind.

> >    if test ! -f "$at_check_line_file"; then
> > -    sed "s/^ */$as_me: warning: /" <<_ATEOF
> > -   A failure happened in a test group before any test could be
> > -   run. This means that test suite is improperly designed.  Please
> > -   report this failure to <AT_PACKAGE_BUGREPORT>.
> > +    sed "s/^ */$as_me: WARNING: /" <<_ATEOF
> 
> Are we consistent on capitalization of warning: vs. WARNING:?

I think this change makes it consistent with the rest of Autoconf, yes.

> > +  echo "$at_res" > "$at_job_dir/$at_res"
> 
> So both the name of the file and the contents of the file describe the result 
> of the test.

Yep.  I thought of
  echo dummy > "$at_job_dir/$at_res"

bug figured that would look, erm, dumby.  Apologies for the pun.  ;-)

> > +# Wrap up the test suite with summary statistics.
> > +cd "$at_helper_dir"
> > +
> > +at_pass_list=`echo */pass | sed 's,/pass,,g; s,\*,,'`
> 
> But here, you are only using the file name, and with several forks to compute 
> the summary.  What about these alternatives?
> 
> 1. Keep the file name as the result, but the contents become the group id.  
> This avoids a pipe, but still forks:
> 
> echo "$at_group" > "$at_job_dir/$at_res"
> ...
> at_pass_list=`cat /dev/null */pass 2>/dev/null`

This needs to read hundreds of files; which can easily be slower than a
fork.  For example, with warm caches, on NFS your command takes 100ms,
while mine takes 8ms.  The cold cache cases are a lot worse, of course.
I'd be interested to hear whether things are different on w32.

More generally, counting forks is not the only thing for efficiency;
I/O is expensive, too.

A minor point is that the above introduces newlines into the variables
(no problem for pass but for xpass and the others).  Of course, they can
easily be killed on the go with
  set X $at_xpass_list; shift; at_xpass_count=$#; at_xpass_list=$*

> 2. Keep the file name as the result but with an exploitable prefix, contents 
> are irrelevant.  Use a shell loop to compute the lists.  Collecting the 
> summary 
> avoids forks altogether if we can then use an XSI construct, but is more 
> expensive on deficient shells (implementation of the two versions of 
> at_func_dirname left to the reader, but see libtool for hints):
> 
> touch "$at_job_dir/result-$at_res"
> ...
> at_pass_list=
> ...
> for at_result in */result-*; do
>   at_func_dirname $at_result
>   at_group=$at_func_dirname_result
>   case $at_result in
>     *-pass) at_pass_list="$at_pass_list $at_group" ;;

This scales quadratically in the number of passed tests.
You can use func_append from Libtool, but only bash has +=.

The fact that this forks once per test for slow shells makes
this really unattractive.

> ...
>   esac
> done

> 3. Like 2, but rework the loop variable to avoid needing at_func_dirname:
> 
> touch "$at_job_dir/result-$at_res"
> ...
> at_pass_list=
> ...
> for at_group in $at_groups; do
>   case $at_group/result-* in

The word after 'case' does not undergo filename expansion.
That can be fixed, but still it would result in one filename
expansion step be done for each test.  I think that is slightly
slower than doing just one filename expansion over all files.

>     *-pass) at_pass_list="$at_pass_list $at_group" ;;

Still quadratic in number of passed tests.

> ...
>   esac
> done

Cheers,
Ralf