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

[Ralf Wildenhues]

* Eric Blake wrote on Fri, May 30, 2008 at 12:27:54AM CEST:
> Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes:
> > > 
> > > Are we consistent on capitalization of warning: vs. WARNING:?
> > 
> > I think this change makes it consistent with the rest of Autoconf, yes.
> 
> Aha.  AS_MESSAGE does indeed print in all caps.  I wonder, though, if that is 
> a 
> violation of GNU Coding Standards.

FWIW, I didn't find anything to that end in GCS.

> At any rate, cleaning it up would be a 
> separate followon patch, and shouldn't hold up the current commit.

OK.

> > > 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.
> 
> Good point.  I didn't really test, so I don't have numbers to back this up; I 
> was just typing as I thought about it, trying to see if it was possible to 
> get 
> a speedup.  Therefore, don't take this particular criticism as a rejection of 
> your approach, so much as a thought experiment to see if there are any 
> possible 
> improvements (and there might not be).

OK.  It's good to have different ideas (and really I didn't take them as
criticism at all).  I probably should've expanded on the fact that I did
test some variants here before arriving at this.

> At any rate, your approach is already an improvement for non-XSI shells, as 
> there are fewer expr calls to work around the lack of $(()).

Yep.

> Can anybody think of a non-forking option 4?  Use five 'test -f' instead of 
> a 'case'?  Otherwise, I'm inclined to concede that your approach is probably 
> the fastest across all shells (only five list varibles to populate, so O(1) 
> pathname expansions sed invocations regardless of number of test groups; and 
> list creation via a single pathname expansion instead of quadratic variable 
> reassignment).  You are right that anything that has to read the files 
> (rather 
> than just using readdir() or access() to test for their existence) will 
> probably be slower.  On the other hand, since each file lives in a different 
> directory, you are still hitting a lot of inodes to to the pathname expansion.

Yep.  Actually, I really hesitated with the idea of having yet another
per-testgroup-directory at all, because that would introduce so many
more inodes into the whole game.  But I didn't really see a way to
achieve this.

Anyway, for optimizing Autotest all forks/file accesses that are done
per-testgroup are more important than those done only a constant number
of times per testsuite run, so I will concentrate on those.


Darn, I just found a couple of regressions that this patch causes that I
didn't think of yet: it will output test numbers of failed tests in the
wrong order:
   Subject: [GNU Autoconf 2.62XXX] testsuite:1 10 3 4 5 6 7 8 9 failed

Also, the sed 's,/pass,,g ...' script may easily receive input lines
which are too long for some sed implementations.  And the spacing in the
above Subject: line is wrong, too.

So below is a patch to fixup this patch (to be squashed).  It begs the
question whether it was a good idea in the first place to have
directories named one way in at-groups and the other for the tests.
I wouldn't mind both being one way or the other, but a change to this
end should be measured (and a change for the toplevel ones obviously is
a user-visible change not to be taken lightly).

Cheers,
Ralf


diff --git a/lib/autotest/general.m4 b/lib/autotest/general.m4
index de575eb..527a392 100644
--- a/lib/autotest/general.m4
+++ b/lib/autotest/general.m4
@@ -1051,17 +1051,17 @@ 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
+  at_check_line_file=$at_job_dir/check-line
   # The file containing the exit status of the last command.
-  at_status_file=$at_job_dir/at-status
+  at_status_file=$at_job_dir/status
   # The files containing the output of the tested commands.
-  at_stdout=$at_job_dir/at-stdout
-  at_stder1=$at_job_dir/at-stder1
-  at_stderr=$at_job_dir/at-stderr
+  at_stdout=$at_job_dir/stdout
+  at_stder1=$at_job_dir/stder1
+  at_stderr=$at_job_dir/stderr
   # The file containing the code for a test group.
-  at_test_source=$at_job_dir/at-test-source
+  at_test_source=$at_job_dir/test-source
   # The file containing dates.
-  at_times_file=$at_job_dir/at-times
+  at_times_file=$at_job_dir/times
 
   # Be sure to come back to the top test directory.
   cd "$at_suite_dir"
@@ -1295,17 +1295,19 @@ fi
 # Wrap up the test suite with summary statistics.
 cd "$at_helper_dir"
 
-at_pass_list=`echo */pass | sed 's,/pass,,g; s,\*,,'`
-at_xpass_list=`echo */xpass | sed 's,/xpass,,g; s,\*,,'`
-at_xfail_list=`echo */xfail | sed 's,/xfail,,g; s,\*,,'`
-at_fail_list=`echo */fail | sed 's,/fail,,g; s,\*,,'`
-at_skip_list=`echo */skip | sed 's,/skip,,g; s,\*,,'`
+at_pass_list=`for f in */pass; do echo $f; done | sed '/*/d; s,/pass,,'`
+at_skip_list=`for f in */skip; do echo $f; done | sed '/*/d; s,/skip,,'`
+at_xfail_list=`for f in */xfail; do echo $f; done | sed '/*/d; s,/xfail,,'`
+at_xpass_list=`for f in ?/xpass ??/xpass ???/xpass ????/xpass; do
+                echo $f; done | sed '/?/d; s,/xpass,,'`
+at_fail_list=`for f in ?/fail ??/fail ???/fail ????/fail; do
+               echo $f; done | sed '/?/d; s,/fail,,'`
 
 set X $at_pass_list $at_xpass_list $at_xfail_list $at_fail_list $at_skip_list
 shift; address@hidden:@]
-set X $at_xpass_list; shift; address@hidden:@]
+set X $at_xpass_list; shift; address@hidden:@]; at_xpass_list=$[*]
 set X $at_xfail_list; shift; address@hidden:@]
-set X $at_fail_list; shift; address@hidden:@]
+set X $at_fail_list; shift; address@hidden:@]; at_fail_list=$[*]
 set X $at_skip_list; shift; address@hidden:@]
 
 at_func_arith $at_group_count - $at_skip_count
@@ -1471,8 +1473,8 @@ else
 [and all information you think might help:
 
    To: <AT_PACKAGE_BUGREPORT>
-   Subject: @<:@AT_PACKAGE_STRING@:>@ $as_me:dnl
-$at_fail_list${at_fail_list:+ failed${at_xpass_list:+,}}dnl
+   Subject: @<:@AT_PACKAGE_STRING@:>@ $as_me: dnl
+$at_fail_list${at_fail_list:+ failed${at_xpass_list:+, }}dnl
 $at_xpass_list${at_xpass_list:+ passed unexpectedly}
 "])
   if test $at_debug_p = false; then