GNOME Bugzilla – Bug 743063
validate: launcher: Add option to run tests in parallel
Last modified: 2015-02-16 16:31:05 UTC
Most tests use just one core or just wait idly for a long time. Running tests in parallel speeds up the whole process, even in a single-core machine, since some tests just remain idle. These results show a good speedup on my box by using 4 cores (I had some unrelated problem with streaming so I disabled http and hls tests): $ gst-validate-launcher --mute -b "validate.hls.*" -b "validate.http.*" -b "ges.*" -j 1 Total time spent: 0:26:31.695281 seconds $ gst-validate-launcher --mute -b "validate.hls.*" -b "validate.http.*" -b "ges.*" -j 4 Total time spent: 0:09:40.981110 seconds Plus if you run the tests without --mute you get a nice symphony =) The code can be found here: http://cgit.collabora.com/git/user/ramiro/gst-devtools.git/log/?h=parallel
Thanks for that, I will have a look soon! About the network tests not working, it is because the server is not able to handle more than one connection at a time (it is a very basic server!). Could you have a look and also fix that?
Added support for multiple threads in the HTTP server. Updated git repo. I still disabled a few tests that take 10 minutes to reach the hard timeout. The GES tests aren't running on git HEAD either. $ gst-validate-launcher --sync --mute -b "validate.http.transcode.*.vorbis_vp8_1_webm" -b validate.http.playback.seek_with_stop.mp3_h264_1_mp4 -b validate.http.playback.seek_with_stop.raw_h264_1_mp4 The results on a box with 4 cores were: -j1 Total time spent: 0:37:03.168778 seconds -j2 Total time spent: 0:20:21.908862 seconds -j4 Total time spent: 0:10:57.954698 seconds -j8 Total time spent: 0:06:44.182272 seconds
(In reply to comment #2) > Added support for multiple threads in the HTTP server. Updated git repo. I > still disabled a few tests that take 10 minutes to reach the hard timeout. We should probably change that hard timeout to something more sensible than 10 minutes. > The GES tests aren't running on git HEAD either. What do you mean GES tests aren't running on master? > $ gst-validate-launcher --sync --mute -b > "validate.http.transcode.*.vorbis_vp8_1_webm" -b > validate.http.playback.seek_with_stop.mp3_h264_1_mp4 -b > validate.http.playback.seek_with_stop.raw_h264_1_mp4 > > The results on a box with 4 cores were: > -j1 Total time spent: 0:37:03.168778 seconds > -j2 Total time spent: 0:20:21.908862 seconds > -j4 Total time spent: 0:10:57.954698 seconds > -j8 Total time spent: 0:06:44.182272 seconds Thanks for that, I will review it as soon as I find some time
Created attachment 294981 [details] [review] validate: launcher: Separate Reporter from current Test Instead of saving the current Test in Reporter for every test, use function parameters to achieve the same goal. Patch 2/5 to move logfile handling out of Reporter and into Test.
Created attachment 294982 [details] [review] validate: launcher: Split test log file handling in Reporter Patch 3/5 to move logfile handling out of Reporter and into Test.
Created attachment 294983 [details] [review] validate: launcher: Remove redundant check self.out is always available when _get_captured() is called. Patch 4/5 to move logfile handling out of Reporter and into Test.
Created attachment 294984 [details] [review] validate: launcher: Move logfile handling out of Reporter and into Test This makes each Test handle its own logfile, allowing the Reporter to work on multiple tests at the same time. Patch 5/5 to move logfile handling out of Reporter and into Test.
Created attachment 294985 [details] [review] validate: launcher: Split process_update() out of wait_process() Patch 1/4 to make TestManager handle waiting for processes instead of expecting each Test to do it.
Created attachment 294986 [details] [review] validate: launcher: Initialize Test start time outside of wait_process wait_process will be moved to TestManager, so the values used to track process update must remain inside Test. Patch 2/4 to make TestManager handle waiting for processes instead of expecting each Test to do it.
Created attachment 294987 [details] [review] validate: launcher: Use a Queue to test for test completion TestManager will use a Queue to track progress for all tests. This commit implements a queue inside Test to simplify the transition. Patch 3/4 to make TestManager handle waiting for processes instead of expecting each Test to do it.
Created attachment 294988 [details] [review] validate: launcher: Make TestManager handle waiting for processes Patch 4/4 to make TestManager handle waiting for processes instead of expecting each Test to do it.
Created attachment 294989 [details] [review] validate: launcher: Support simultaneous requests in RangeHTTPServer
Created attachment 294990 [details] [review] validate: launcher: Use test index instead of counting test numbers Patch 1/4 to implement parallel test execution.
Created attachment 294991 [details] [review] validate: launcher: Use jobs list to take track or tests running Currently the tests are still run serially. Patch 2/4 to implement parallel test execution.
Created attachment 294992 [details] [review] validate: launcher: Print test number on result With parallel test execution, it will be hard to track which result relates to which test. Therefore, the test number should be printed along with the results as well. Patch 3/4 to implement parallel test execution.
Created attachment 294993 [details] [review] validate: launcher: Add option to run tests in parallel Patch 4/4 to implement parallel test execution.
Review of attachment 294981 [details] [review]: Looks good
Review of attachment 294982 [details] [review]: Not sure what problem that solves, but looks OK
Review of attachment 294983 [details] [review]: OK
Review of attachment 294984 [details] [review]: OK
Review of attachment 294985 [details] [review]: OK
Review of attachment 294988 [details] [review]: Looks good ::: validate/launcher/baseclasses.py @@ +795,3 @@ def run_tests(self, cur_test_num, total_num_tests): + self.queue = Queue.Queue() + Would be better to put in __init__ but, no big deal
Review of attachment 294989 [details] [review]: Impressively simple :)
Review of attachment 294993 [details] [review]: Nicely simple :) ::: validate/launcher/main.py @@ +430,3 @@ help="Redirect logs to 'stdout' or 'sdterr'.") + dir_group.add_argument("-j", "--jobs", dest="num_jobs", + help="Number of tests to execute simultaneously") You could put type=int instead of checking the type yourself.
Review of attachment 294992 [details] [review]: As I said in another commit I think we should print the classname of the test too.
Review of attachment 294991 [details] [review]: Looks good in overall ::: validate/launcher/baseclasses.py @@ +798,3 @@ except KeyboardInterrupt: + for test in self.jobs: + test._kill_subprocess() It should be called kill_subprcess if it is public
Review of attachment 294990 [details] [review]: ::: validate/launcher/baseclasses.py @@ +814,3 @@ + def print_test_num(self, test): + cur_test_num = self.starting_test_num + self.tests.index(test) + 1 + sys.stdout.write("[%d / %d] " % (cur_test_num, self.total_num_tests)) I think we should also print the test class here (would look better)
Review of attachment 294987 [details] [review]: OK
Review of attachment 294986 [details] [review]: OK
Review of attachment 294992 [details] [review]: OK
Review of attachment 294991 [details] [review]: OK
Review of attachment 294982 [details] [review]: after_test() currently has two functions: add results and close log file. This split makes it simpler to move logfile handling out of Reporter and into Test in the following patches. Adding results remains in after_test(), but the rest of the code is made specific to logfile handling.
Review of attachment 294988 [details] [review]: ::: validate/launcher/baseclasses.py @@ +795,3 @@ def run_tests(self, cur_test_num, total_num_tests): + self.queue = Queue.Queue() + Changed. It affected other patches, so they'll be updated as well.
Review of attachment 294990 [details] [review]: ::: validate/launcher/baseclasses.py @@ +814,3 @@ + def print_test_num(self, test): + cur_test_num = self.starting_test_num + self.tests.index(test) + 1 + sys.stdout.write("[%d / %d] " % (cur_test_num, self.total_num_tests)) I'm adding a new patch for this (validate: launcher: Print test name in Result). It can be applied independently from this patchset.
Review of attachment 294991 [details] [review]: ::: validate/launcher/baseclasses.py @@ +798,3 @@ except KeyboardInterrupt: + for test in self.jobs: + test._kill_subprocess() Fixed.
Review of attachment 294993 [details] [review]: Thanks, that's simpler. ::: validate/launcher/main.py @@ +430,3 @@ help="Redirect logs to 'stdout' or 'sdterr'.") + dir_group.add_argument("-j", "--jobs", dest="num_jobs", + help="Number of tests to execute simultaneously") Thanks, that's simpler.
Created attachment 295013 [details] [review] validate: launcher: Use a Queue to test for test completion
Created attachment 295014 [details] [review] validate: launcher: Make TestManager handle waiting for processes
Created attachment 295015 [details] [review] validate: launcher: Use test index instead of counting test numbers
Created attachment 295016 [details] [review] validate: launcher: Use jobs list to take track of tests running
Created attachment 295017 [details] [review] validate: launcher: Add option to run tests in parallel
Created attachment 295018 [details] [review] validate: launcher: Print test name in Result
Review of attachment 295018 [details] [review]: OK
Review of attachment 295013 [details] [review]: OK
Review of attachment 295014 [details] [review]: OK
Review of attachment 295015 [details] [review]: OK
Review of attachment 295016 [details] [review]: OK
Review of attachment 295017 [details] [review]: OK
Attachment 294987 [details] pushed as 0026c28 - validate: launcher: Use a Queue to test for test completion Attachment 294989 [details] pushed as 9c46f5f - validate: launcher: Support simultaneous requests in RangeHTTPServer Attachment 295013 [details] pushed as 0026c28 - validate: launcher: Use a Queue to test for test completion Attachment 295014 [details] pushed as ef246a4 - validate: launcher: Make TestManager handle waiting for processes Attachment 295015 [details] pushed as d9097f8 - validate: launcher: Use test index instead of counting test numbers Attachment 295016 [details] pushed as a9a3664 - validate: launcher: Use jobs list to take track of tests running Attachment 295017 [details] pushed as c0cefec - validate: launcher: Add option to run tests in parallel Attachment 295018 [details] pushed as d833a51 - validate: launcher: Print test name in Result