| Message ID | 1483673581-7843-3-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 06, 2017 at 11:33:01AM +0800, Peter Xu wrote: > run_task.sh is getting slow. This patch is trying to make it faster by > running the tests concurrently. > > We provide a new parameter "-j" for the run_tests.sh, which can be used > to specify how many run queues we want for the tests. Default queue > length is 1, which is the old behavior. > > Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost: > > |-----------------+-----------| > | command | time used | > |-----------------+-----------| > | run_test.sh | 75s | > | run_test.sh -j8 | 27s | > |-----------------+-----------| > > Suggested-by: Radim Krčmář <rkrcmar@redhat.com> Radim suggested how to implement this, but not the idea of implementing it. The suggested-by tag is for ideas, not the code, and the idea (which is a good one) was yours, not Radim's. So the above tag should be removed. It's hard to credit Radim for his help here. Adding a signed-off-by to indicate co-authorship is probably the best you can do. Of course he's the maintainer and will add that when he merges anyway though... > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > run_tests.sh | 12 ++++++++++-- > scripts/functions.bash | 17 ++++++++++++++++- > scripts/global.bash | 11 +++++++++++ > 3 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/run_tests.sh b/run_tests.sh > index e1bb3a6..a4fc895 100755 > --- a/run_tests.sh > +++ b/run_tests.sh > @@ -14,10 +14,11 @@ function usage() > { > cat <<EOF > > -Usage: $0 [-g group] [-h] [-v] > +Usage: $0 [-g group] [-h] [-v] [-j N] s/N/num_run_queues/ > > -g: Only execute tests in the given group > -h: Output this help text > + -j: Execute tests in parallel > -v: Enables verbose mode > > Set the environment variable QEMU=/path/to/qemu-system-ARCH to > @@ -29,7 +30,7 @@ EOF > RUNTIME_arch_run="./$TEST_DIR/run" > source scripts/runtime.bash > > -while getopts "g:hv" opt; do > +while getopts "g:hj:v" opt; do > case $opt in > g) > only_group=$OPTARG > @@ -38,6 +39,13 @@ while getopts "g:hv" opt; do > usage > exit > ;; > + j) > + ut_run_queues=$OPTARG > + if ! is_number "$ut_run_queues"; then > + echo "Invalid -j option: $ut_run_queues" > + exit 1 > + fi Instead of adding is_number() and just checking for numeric input, I'd check the input is > 0. A string input resolves as zero when treated as a number, so it'll fail too. > + ;; > v) > verbose="yes" > ;; > diff --git a/scripts/functions.bash b/scripts/functions.bash > index d1d2e1c..c6281f4 100644 > --- a/scripts/functions.bash > +++ b/scripts/functions.bash > @@ -1,9 +1,18 @@ > +source scripts/global.bash > + > function run_task() > { > local testname="$2" > > + while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do Bash arithmetic expressions should use (( ... )) instead of [[ ... ]] > + # wait for any background test to finish > + wait -n > + done > + > RUNTIME_log_file="${ut_log_dir}/${testname}.log" > - "$@" > + > + # start the testcase in the background > + "$@" & > } > > function for_each_unittest() > @@ -20,6 +29,8 @@ function for_each_unittest() > local accel > local timeout > > + trap "wait; exit 130" SIGINT > + > exec {fd}<"$unittests" > > while read -u $fd line; do > @@ -53,5 +64,9 @@ function for_each_unittest() > fi > done > run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + > + # wait all task finish > + wait > + > exec {fd}<&- > } > diff --git a/scripts/global.bash b/scripts/global.bash > index 77b0b29..52095bd 100644 > --- a/scripts/global.bash > +++ b/scripts/global.bash > @@ -1,2 +1,13 @@ > : ${ut_log_dir:=logs} > : ${ut_log_summary:=${ut_log_dir}/SUMMARY} > +: ${ut_run_queues:=1} > + > +function ut_in_parallel() > +{ > + [[ $ut_run_queues != 1 ]] > +} I don't see any users of ut_in_parallel. Anyway I'd drop it and open code the condition, with ((...)), when needed. > + > +function is_number() > +{ > + [[ "$1" =~ ^[0-9]+$ ]] > +} > -- > 2.7.4 > > Thanks, drew
On Fri, Jan 06, 2017 at 03:40:41PM +0100, Andrew Jones wrote: > On Fri, Jan 06, 2017 at 11:33:01AM +0800, Peter Xu wrote: > > run_task.sh is getting slow. This patch is trying to make it faster by > > running the tests concurrently. > > > > We provide a new parameter "-j" for the run_tests.sh, which can be used > > to specify how many run queues we want for the tests. Default queue > > length is 1, which is the old behavior. > > > > Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost: > > > > |-----------------+-----------| > > | command | time used | > > |-----------------+-----------| > > | run_test.sh | 75s | > > | run_test.sh -j8 | 27s | > > |-----------------+-----------| > > > > Suggested-by: Radim Krčmář <rkrcmar@redhat.com> > > Radim suggested how to implement this, but not the idea of implementing > it. The suggested-by tag is for ideas, not the code, and the idea (which > is a good one) was yours, not Radim's. So the above tag should be removed. > It's hard to credit Radim for his help here. Adding a signed-off-by to > indicate co-authorship is probably the best you can do. Of course he's > the maintainer and will add that when he merges anyway though... I see, thanks for the clarification. Then let me remove this line. > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > run_tests.sh | 12 ++++++++++-- > > scripts/functions.bash | 17 ++++++++++++++++- > > scripts/global.bash | 11 +++++++++++ > > 3 files changed, 37 insertions(+), 3 deletions(-) > > > > diff --git a/run_tests.sh b/run_tests.sh > > index e1bb3a6..a4fc895 100755 > > --- a/run_tests.sh > > +++ b/run_tests.sh > > @@ -14,10 +14,11 @@ function usage() > > { > > cat <<EOF > > > > -Usage: $0 [-g group] [-h] [-v] > > +Usage: $0 [-g group] [-h] [-v] [-j N] > > s/N/num_run_queues/ Sure. [...] > > @@ -38,6 +39,13 @@ while getopts "g:hv" opt; do > > usage > > exit > > ;; > > + j) > > + ut_run_queues=$OPTARG > > + if ! is_number "$ut_run_queues"; then > > + echo "Invalid -j option: $ut_run_queues" > > + exit 1 > > + fi > > Instead of adding is_number() and just checking for numeric > input, I'd check the input is > 0. A string input resolves > as zero when treated as a number, so it'll fail too. Wasn't familiar with shell arithmetic before. Your suggestion is much simpler, thanks. > > > + ;; > > v) > > verbose="yes" > > ;; > > diff --git a/scripts/functions.bash b/scripts/functions.bash > > index d1d2e1c..c6281f4 100644 > > --- a/scripts/functions.bash > > +++ b/scripts/functions.bash > > @@ -1,9 +1,18 @@ > > +source scripts/global.bash > > + > > function run_task() > > { > > local testname="$2" > > > > + while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do > > Bash arithmetic expressions should use (( ... )) instead > of [[ ... ]] Will do. [...] > > diff --git a/scripts/global.bash b/scripts/global.bash > > index 77b0b29..52095bd 100644 > > --- a/scripts/global.bash > > +++ b/scripts/global.bash > > @@ -1,2 +1,13 @@ > > : ${ut_log_dir:=logs} > > : ${ut_log_summary:=${ut_log_dir}/SUMMARY} > > +: ${ut_run_queues:=1} > > + > > +function ut_in_parallel() > > +{ > > + [[ $ut_run_queues != 1 ]] > > +} > > I don't see any users of ut_in_parallel. Anyway I'd drop it > and open code the condition, with ((...)), when needed. Yes, it should be removed. Thanks! -- peterx
diff --git a/run_tests.sh b/run_tests.sh index e1bb3a6..a4fc895 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -14,10 +14,11 @@ function usage() { cat <<EOF -Usage: $0 [-g group] [-h] [-v] +Usage: $0 [-g group] [-h] [-v] [-j N] -g: Only execute tests in the given group -h: Output this help text + -j: Execute tests in parallel -v: Enables verbose mode Set the environment variable QEMU=/path/to/qemu-system-ARCH to @@ -29,7 +30,7 @@ EOF RUNTIME_arch_run="./$TEST_DIR/run" source scripts/runtime.bash -while getopts "g:hv" opt; do +while getopts "g:hj:v" opt; do case $opt in g) only_group=$OPTARG @@ -38,6 +39,13 @@ while getopts "g:hv" opt; do usage exit ;; + j) + ut_run_queues=$OPTARG + if ! is_number "$ut_run_queues"; then + echo "Invalid -j option: $ut_run_queues" + exit 1 + fi + ;; v) verbose="yes" ;; diff --git a/scripts/functions.bash b/scripts/functions.bash index d1d2e1c..c6281f4 100644 --- a/scripts/functions.bash +++ b/scripts/functions.bash @@ -1,9 +1,18 @@ +source scripts/global.bash + function run_task() { local testname="$2" + while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do + # wait for any background test to finish + wait -n + done + RUNTIME_log_file="${ut_log_dir}/${testname}.log" - "$@" + + # start the testcase in the background + "$@" & } function for_each_unittest() @@ -20,6 +29,8 @@ function for_each_unittest() local accel local timeout + trap "wait; exit 130" SIGINT + exec {fd}<"$unittests" while read -u $fd line; do @@ -53,5 +64,9 @@ function for_each_unittest() fi done run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + + # wait all task finish + wait + exec {fd}<&- } diff --git a/scripts/global.bash b/scripts/global.bash index 77b0b29..52095bd 100644 --- a/scripts/global.bash +++ b/scripts/global.bash @@ -1,2 +1,13 @@ : ${ut_log_dir:=logs} : ${ut_log_summary:=${ut_log_dir}/SUMMARY} +: ${ut_run_queues:=1} + +function ut_in_parallel() +{ + [[ $ut_run_queues != 1 ]] +} + +function is_number() +{ + [[ "$1" =~ ^[0-9]+$ ]] +}
run_task.sh is getting slow. This patch is trying to make it faster by running the tests concurrently. We provide a new parameter "-j" for the run_tests.sh, which can be used to specify how many run queues we want for the tests. Default queue length is 1, which is the old behavior. Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost: |-----------------+-----------| | command | time used | |-----------------+-----------| | run_test.sh | 75s | | run_test.sh -j8 | 27s | |-----------------+-----------| Suggested-by: Radim Krčmář <rkrcmar@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- run_tests.sh | 12 ++++++++++-- scripts/functions.bash | 17 ++++++++++++++++- scripts/global.bash | 11 +++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-)