RESOLVED FIXED 190190
Allow run-javascriptcore-tests to distribute tests between devices
https://bugs.webkit.org/show_bug.cgi?id=190190
Summary Allow run-javascriptcore-tests to distribute tests between devices
Guillaume Emont
Reported 2018-10-02 01:02:13 PDT
Some devices just take a lot of time to run all our tests. A way around it which we want to use for the EWS is to spread the tests over several devices.
Attachments
Patch (11.37 KB, patch)
2018-10-03 05:13 PDT, Guillaume Emont
no flags
Patch (11.29 KB, patch)
2018-10-03 08:51 PDT, Guillaume Emont
no flags
Patch (11.25 KB, patch)
2018-10-04 06:27 PDT, Guillaume Emont
no flags
Guillaume Emont
Comment 1 2018-10-03 05:13:05 PDT
Created attachment 351511 [details] Patch WIP first version of the patch, to try on the EWS.
Dominik Inführ
Comment 2 2018-10-03 07:58:41 PDT
Comment on attachment 351511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351511&action=review > Tools/Scripts/run-jsc-stress-tests:268 > + $remoteHosts = [ { :name => 'default', Could we use `Struct.new` here? It should the be possible to invoke `remoteHost.remoteDirectory` instead of `remoteHost[:remoteDirectory]` > Tools/Scripts/run-jsc-stress-tests:1792 > + remoteHost=$remoteHosts[remoteIndex] spaces around `=` > Tools/Scripts/run-jsc-stress-tests:1908 > + remoteHost=$remoteHosts[remoteIndex] spaces around `=`
Guillaume Emont
Comment 3 2018-10-03 08:51:41 PDT
Created attachment 351525 [details] Patch WIP new version using a Struct and fixing missing spaces
Dominik Inführ
Comment 4 2018-10-03 12:07:03 PDT
Comment on attachment 351525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351525&action=review > Tools/Scripts/run-jsc-stress-tests:233 > + $remoteHosts << RemoteHost.new("default-#{$remoteHosts.length.to_s}", uri.user, uri.host, uri.port) `.to_s` shouldn't be necessary here. > Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:338 > + File.open($runnerDir + "Makefile.#{remoteIndex.to_s}", "w") { `.to_s` shouldn't be necessary here. > Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:365 > + command = "make -j #{$numChildProcesses.to_s} -s -f Makefile.#{remoteIndex.to_s}" `.to_s` shouldn't be necessary here. > Tools/Scripts/webkitruby/jsc-stress-test-writer-ruby.rb:410 > + File.open($runnerDir + "Makefile.#{remoteIndex.to_s}", "w") { `.to_s` shouldn't be necessary here. > Tools/Scripts/webkitruby/jsc-stress-test-writer-ruby.rb:437 > + command = "make -j #{$numChildProcesses.to_s} -s -f Makefile.#{remoteIndex.to_s}" ditto
Guillaume Emont
Comment 5 2018-10-04 06:27:46 PDT
Created attachment 351595 [details] Patch WIP now without unneeded .to_s
Guillaume Emont
Comment 6 2018-10-04 10:14:34 PDT
I can confirm that this patch is useful: using 3 Raspberry Pi 3 devices, run-javascriptcore-tests takes just under 45 mins to run tests in armv7 (our buildbot currently takes about 2h15 to do that with only one device).
Guillaume Emont
Comment 7 2018-10-05 03:36:20 PDT
Comment on attachment 351595 [details] Patch I think at this point we can consider the patch to be ready for a proper review.
Adrian Perez
Comment 8 2018-10-05 09:02:42 PDT
Comment on attachment 351595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351595&action=review Informally reviewing… patch LGTM, with a comment/suggestion for a follow-up patch. > Tools/Scripts/run-jsc-stress-tests:321 > +$progressMeter = ($verbosity == 0 and $stdout.tty? and $remoteHosts.length <= 1) It would be nicer to have some kind of progress meter also in multiple remote host mode, but I understand it may complicate the script further, due to having to track the progress for each remote host separately and then aggregate them for display. Do you have any thoughts about this? I wouldn't block landing the patch on this (the functionality *is* a good thing for parallelizing and running tests faster, and faster testing is always better!), but I think a follow-up patch that adds support for progress report with multiple remotes would be valuable.
Guillaume Emont
Comment 9 2018-10-05 09:11:06 PDT
(In reply to Adrian Perez from comment #8) > Comment on attachment 351595 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351595&action=review > > Informally reviewing… patch LGTM, with a comment/suggestion > for a follow-up patch. > > > Tools/Scripts/run-jsc-stress-tests:321 > > +$progressMeter = ($verbosity == 0 and $stdout.tty? and $remoteHosts.length <= 1) > > It would be nicer to have some kind of progress meter also in multiple > remote host mode, but I understand it may complicate the script further, > due to having to track the progress for each remote host separately and > then aggregate them for display. Do you have any thoughts about this? I thought about it, but then realized that this would greatly increase the scope of this patch, and I wanted to keep it small since I want to land this feature soon since it is needed to deploy our armv7 EWS ...and the EWS won't benefit from this anyway, as the progress meter only makes sense when running tests "manually". > > I wouldn't block landing the patch on this (the functionality *is* a > good thing for parallelizing and running tests faster, and faster testing > is always better!), but I think a follow-up patch that adds support for > progress report with multiple remotes would be valuable. I agree.
Michael Catanzaro
Comment 10 2018-10-07 12:59:49 PDT
Ideally a JSC reviewer would review this one.
Guillaume Emont
Comment 11 2018-10-09 07:29:48 PDT
Ping reviewer.
Michael Catanzaro
Comment 12 2018-10-09 17:01:25 PDT
Comment on attachment 351595 [details] Patch I see no objections. LGTM.
WebKit Commit Bot
Comment 13 2018-10-09 17:28:22 PDT
Comment on attachment 351595 [details] Patch Clearing flags on attachment: 351595 Committed r236993: <https://trac.webkit.org/changeset/236993>
WebKit Commit Bot
Comment 14 2018-10-09 17:28:24 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2018-10-09 17:29:24 PDT
Note You need to log in before you can comment on or make changes to this bug.