On Mon, Jun 19, 2023 at 01:50:58PM +0300, Amir Goldstein wrote:
> kvm-xfstests invokes check -n twice to pre-process and generate the
> tests-to-run list, which is then being passed as a long list of tests
> for invkoing check in the command line.
>
> check invokes dirname, basename and sed several times per test just
> for doing basic string prefix/suffix trimming.
> Use bash string pattern matching instead which is much faster.
>
> Note that the following pattern matching expression change:
> < test_dir=${test_dir#$SRC_DIR/*}
> > t=${t#$SRC_DIR/}
> does not change the meaning of the expression, because the
> shortest match of "$SRC_DIR/*" that is being trimmed is "$SRC_DIR/"
> and removing the tests/ prefix is what this code intended to do.
>
> With check -n, there is no need to cleanup the results dir,
> but check -n is doing that for every single listed test.
> Move the cleanup of results dir to before actually running the test.
>
> These improvements to check pre-test code cut down several minutes
> from the time until tests actually start to run with kvm-xfstests.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> Zorro,
>
> Just to clarify, this change is not expected to change behavior -
> only to improve runtime of check -n and the things that happen
> before the first test is invoked.
>
> Thanks,
> Amir.
>
> check | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/check b/check
> index e36978c1..1a16eccb 100755
> --- a/check
> +++ b/check
> @@ -399,9 +399,9 @@ if $have_test_arg; then
> *) # Expand test pattern (e.g. xfs/???, *fs/001)
> list=$(cd $SRC_DIR; echo $1)
> for t in $list; do
> - test_dir=`dirname $t`
> - test_dir=${test_dir#$SRC_DIR/*}
> - test_name=`basename $t`
> + t=${t#$SRC_DIR/}
Actually I'm not sure what's this line for. We get $list by "cd $SRC_DIR; echo $1",
is there any chance to have "$SRC_DIR" in $t (or $test_dir) ?
> + test_dir=${t%/*}
> + test_name=${t#*/}
This change looks isn't related with the "check -n". Any reason to change
that? Does the 'dirname' and 'basename' have poor performance, or some systems
miss them?
I prefer the 'dirname' and 'basename' ways, if there's not a specific reason
to change that. Due to they can deal with more complicated path name, e.g.
"xfs//001". But the "$(cd $SRC_DIR; echo $1)" looks won't generate a path name
likes that, so if you have a proper reason to make this change, that's fine.
> group_file=$SRC_DIR/$test_dir/group.list
>
> if grep -Eq "^$test_name" $group_file; then
> @@ -849,18 +849,14 @@ function run_section()
>
> # the filename for the test and the name output are different.
> # we don't include the tests/ directory in the name output.
> - export seqnum=`echo $seq | sed -e "s;$SRC_DIR/;;"`
> -
> - # Similarly, the result directory needs to replace the tests/
> - # part of the test location.
> - group=`dirname $seq`
> + export seqnum=${seq#$SRC_DIR/}
> + group=${seqnum%/*}
> if $OPTIONS_HAVE_SECTIONS; then
> - export RESULT_DIR=`echo $group | sed -e "s;$SRC_DIR;${RESULT_BASE}/$section;"`
> REPORT_DIR="$RESULT_BASE/$section"
> else
> - export RESULT_DIR=`echo $group | sed -e "s;$SRC_DIR;$RESULT_BASE;"`
> REPORT_DIR="$RESULT_BASE"
> fi
> + export RESULT_DIR="$REPORT_DIR/$group"
Hmm, this code logic looks more concise.
> seqres="$REPORT_DIR/$seqnum"
>
> # Generate the entire section report with whatever test results
> @@ -872,9 +868,6 @@ function run_section()
> "" &> /dev/null
> fi
>
> - mkdir -p $RESULT_DIR
> - rm -f ${RESULT_DIR}/require_scratch*
> - rm -f ${RESULT_DIR}/require_test*
> echo -n "$seqnum"
>
> if $showme; then
> @@ -899,6 +892,9 @@ function run_section()
> fi
>
> # really going to try and run this one
> + mkdir -p $RESULT_DIR
> + rm -f ${RESULT_DIR}/require_scratch*
> + rm -f ${RESULT_DIR}/require_test*
Nice catch, loop running these lines really takes much time.
Thanks,
Zorro
> rm -f $seqres.out.bad $seqres.hints
>
> # check if we really should run it
> --
> 2.34.1
>
