multiple-pause-resume: make running all pipelines default #706
Conversation
keyonjie
left a comment
keyonjie
left a comment
There was a problem hiding this comment.
nice change, a little bit scare the stricter test may report more errors to us.
aiChaoSONG
force-pushed
the
pause_resume
branch
2 times, most recently
from
0916c78 to
fef8237
Compare
If there are too many pipelines on a platform, the test case runs very long, because it will iterate each combination of two out of all pipelines. This patch make running all pipelines the default while still maintaining the ability to catch bugs under multiple pause resume condition Signed-off-by: Chao Song <chao.song@linux.intel.com>
|
SOFCI TEST |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
I find the pipeline logic of this test was difficult to understand before this commit because the English was quite confusing and some of the help and code comments seemed out of sync with the actual code. After this PR the help and comments are still confusing (unchanged) but now they seem even more out of sync with the new code. Please review and fix the -h usage and code comments related to the pipeline logic before changing that logic.
If there are too many pipelines on a platform, the
test case runs very long, because it will iterate
each combination of two out of all pipelines.
That's not what the -h usage said.
# merge all pipeline to the 1 group
What is "the 1 group"?
Signed-off-by: Chao Song <chao.song@linux.intel.com>
| # merge all pipeline to the 1 group | ||
| # Create two arrays to store command and file that will be used when start a pipeline. | ||
| # For playback pipeline, aplay and /dev/zero will be used, for capture pipeline arecord | ||
| # and /dev/null will be used. |
There was a problem hiding this comment.
Nit: having this comment before the declares would help.
| ## fake pause/resume with expect | ||
| ## expect sleep for sleep time then mocks spacebar keypresses ' ' to | ||
| ## cause resume action | ||
| ## Run pipelines in parallel and pause-resume them. |
There was a problem hiding this comment.
Thanks this is so much clearer, now I understand the logic. My main problem was that I didn't understand what sof-combinatoric.py does and you very clearly explained that.
I think this help is still missing just one important thing: it should simply say what the code does: that the value '1' is really just a convenience shortcut for the pipeline count. Right now it sounds like '1' triggers some special logic but it does not, '1' merely sets a value and then the exact same code runs. So it is a bit of special case but nowhere near as much as this sounds.
| ## that runs in parallel, the default value for '-c' is 1, | ||
| ## with which all pipelines will run in parallel and get pause- | ||
| ## resumed. Manually specified value should be greater than 1, | ||
| ## and less than the maximum pipeline count. |
There was a problem hiding this comment.
Manually specified value should be greater than 1, and less than the maximum pipeline count.
That does not seem correct. It is possible and OK to specify 1 or the pipeline count (and they have the same, documented effect)
It is also possible to pass a value greater than the maximum count as you silently cap that to the maximum. Capping is probably OK but that's not what the help above says.
|
SOFCI TEST |
|
Thank you @aiChaoSONG and @marc-hb. Indeed it will save a lot of time. But by default it run with all pipelines, this is meaningful for stress test but too extreme test case by default, isn't it? We don't want to just saving testing time but want to use the time wisely. Here are some inputs/comments from core developers,
|
|
Even if this code change is not accepted we absolutely want these very valuable documentation fixes (with some corrections). |
|
@marc-hb @fredoh9 @aiChaoSONG we want this improvement in urgent to help on the pause/release xrun issue reproduction, but 13 days has passed, can we speed up this please? I don't expect perfect documentation coverage in one shot, but a significant improvement with no obvious regression should really go much faster IMHO. |
|
Sorry, with this change applied, I can't reproduce the TGL_RVP_NOCODEC xrun issue anymore, looks this change remove randomness of the pipelines combination and pause/release sequence. Can we add randomness of the start/stop/pause/release sequence of each pipeline @aiChaoSONG ? It is in fixed pattern with your change here? |
|
let's don't change the default behavior of this test case at the moment to avoid missing the hot xrun issues capture by multiple-pause-resume case. |
Everyone wants significant speed improvements, as you found later the important question here now is about the potentially significant test coverage difference. Does not seem like an easy question, for sure it's not easy for me. The meaning of "No obvious regression" is very different for test code, in fact a test more likely to fail is a better test if it catches more bugs = what saves much, much more time in the end https://deepsource.io/blog/exponential-cost-of-fixing-bugs/ This is a good example of why sof-test is not where it should be. Good test coverage in the shortest amount of time is a very difficult trade-off, especially with limited and unreliable hardware resources[*]. So it requires hard work and expert discussions that take time (and BTW thank you @fredoh9 for leading these discussions right now). Yet many of our experts either never look at sof-test, or when they do they don't spend enough time, misunderstand test code and changes like this one. Inaccurate test documentation contributes to that problem of course. Look at the lack of "diversity" at https://github.com/thesofproject/sof-test/graphs/contributors [*] our emulation is not where it should be either. |
|
The test is called 'multiple-pause-resume' for a reason, the multiple referred to "multiple devices". |
|
we still have xrun on multicore, change the default behavior may hide the issue, let's close this for now. |
There is a combinatorial explosion in this test because it tries by default to test all pairs of available pipelines. This typically blows up and times out in NOCODEC configurations see #706 and others for earlier discussions. Internally, this combinatorial explosion issue was raised as early as November 2020! This tiny commit does not fix the root cause but it makes a dead simple, "quality of life" adjustment: it simply lowers the default number of iterations from 5 to 1. There is no obvious reason why the default should be 5 and every test plan can raise that value back up if it wants to. Unlike #706 which was abandoned because it reduces test coverage, this very small change merely reduces the default test duration without changing any of its logic. Also, log the combination tested. Sample output: ``` declare -a pipeline_combine_lst=([0]="0 1" [1]="0 2" [2]="0 3" [3]="0 4" [4]="0 5" [5]="0 6" [6]="0 7" [7]="0 8" [8]="0 9" [9]="0 10" [10]="1 2" [11]="1 3" [12]="1 4" [13]="1 5" [14]="1 6" [15]="1 7" [16]="1 8" [17]="1 9" [18]="1 10" [19]="2 3" [20]="2 4" [21]="2 5" [22]="2 6" [23]="2 7" [24]="2 8" [25]="2 9" [26]="2 10" [27]="3 4" [28]="3 5" [29]="3 6" [30]="3 7" [31]="3 8" [32]="3 9" [33]="3 10" [34]="4 5" [35]="4 6" [36]="4 7" [37]="4 8" [38]="4 9" [39]="4 10" [40]="5 6" [41]="5 7" [42]="5 8" [43]="5 9" [44]="5 10" [45]="6 7" [46]="6 8" [47]="6 9" [48]="6 10" [49]="7 8" [50]="7 9" [51]="7 10" [52]="8 9" [53]="8 10" [54]="9 10") ``` Signed-off-by: Marc Herbert <marc.herbert@intel.com>
There is a combinatorial explosion in this test because it tries by default to test all pairs of available pipelines. This typically blows up and times out in NOCODEC configurations see thesofproject#706 and others for earlier discussions. Internally, this combinatorial explosion issue was raised as early as November 2020! (Issues 40, 581,...) This tiny commit does not fix the root cause but it makes a dead simple, "quality of life" adjustment: it simply lowers the default number of iterations from 5 to 1. There is no obvious reason why the default should be 5 and every test plan can raise that value back up if it wants to. Unlike thesofproject#706 which was abandoned because it reduces test coverage, this very small change merely reduces the default test duration without changing any of its logic. Also, log the combination tested. Sample output: ``` declare -a pipeline_combine_lst=([0]="0 1" [1]="0 2" [2]="0 3" [3]="0 4" [4]="0 5" [5]="0 6" [6]="0 7" [7]="0 8" [8]="0 9" [9]="0 10" [10]="1 2" [11]="1 3" [12]="1 4" [13]="1 5" [14]="1 6" [15]="1 7" [16]="1 8" [17]="1 9" [18]="1 10" [19]="2 3" [20]="2 4" [21]="2 5" [22]="2 6" [23]="2 7" [24]="2 8" [25]="2 9" [26]="2 10" [27]="3 4" [28]="3 5" [29]="3 6" [30]="3 7" [31]="3 8" [32]="3 9" [33]="3 10" [34]="4 5" [35]="4 6" [36]="4 7" [37]="4 8" [38]="4 9" [39]="4 10" [40]="5 6" [41]="5 7" [42]="5 8" [43]="5 9" [44]="5 10" [45]="6 7" [46]="6 8" [47]="6 9" [48]="6 10" [49]="7 8" [50]="7 9" [51]="7 10" [52]="8 9" [53]="8 10" [54]="9 10") ``` Signed-off-by: Marc Herbert <marc.herbert@intel.com>
There is a combinatorial explosion in this test because it tries by default to test all pairs of available pipelines. This typically blows up and times out in NOCODEC configurations see #706 and others for earlier discussions. Internally, this combinatorial explosion issue was raised as early as November 2020! (Issues 40, 581,...) This tiny commit does not fix the root cause but it makes a dead simple, "quality of life" adjustment: it simply lowers the default number of iterations from 5 to 1. There is no obvious reason why the default should be 5 and every test plan can raise that value back up if it wants to. Unlike #706 which was abandoned because it reduces test coverage, this very small change merely reduces the default test duration without changing any of its logic. Also, log the combination tested. Sample output: ``` declare -a pipeline_combine_lst=([0]="0 1" [1]="0 2" [2]="0 3" [3]="0 4" [4]="0 5" [5]="0 6" [6]="0 7" [7]="0 8" [8]="0 9" [9]="0 10" [10]="1 2" [11]="1 3" [12]="1 4" [13]="1 5" [14]="1 6" [15]="1 7" [16]="1 8" [17]="1 9" [18]="1 10" [19]="2 3" [20]="2 4" [21]="2 5" [22]="2 6" [23]="2 7" [24]="2 8" [25]="2 9" [26]="2 10" [27]="3 4" [28]="3 5" [29]="3 6" [30]="3 7" [31]="3 8" [32]="3 9" [33]="3 10" [34]="4 5" [35]="4 6" [36]="4 7" [37]="4 8" [38]="4 9" [39]="4 10" [40]="5 6" [41]="5 7" [42]="5 8" [43]="5 9" [44]="5 10" [45]="6 7" [46]="6 8" [47]="6 9" [48]="6 10" [49]="7 8" [50]="7 9" [51]="7 10" [52]="8 9" [53]="8 10" [54]="9 10") ``` Signed-off-by: Marc Herbert <marc.herbert@intel.com>
5 participants
If there are too many pipelines on a platform, the
test case runs very long, because it will iterate
each combination of two out of all pipelines.
This patch make running all pipelines the default
while still maintaining the ability to catch bugs
under multiple pause resume condition
Signed-off-by: Chao Song chao.song@linux.intel.com
This patch actually makes the test much more strict because we are running all pipelines and pause/resume them
Without this patch: multiple-pause-resume.sh runs 2m11.594s on GLK.
With this patch: multiple-pause-resume.sh runs 10.171s on GLK