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.
Created attachment 351511 [details] Patch WIP first version of the patch, to try on the EWS.
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 `=`
Created attachment 351525 [details] Patch WIP new version using a Struct and fixing missing spaces
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
Created attachment 351595 [details] Patch WIP now without unneeded .to_s
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).
Comment on attachment 351595 [details] Patch I think at this point we can consider the patch to be ready for a proper review.
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.
(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.
Ideally a JSC reviewer would review this one.
Ping reviewer.
Comment on attachment 351595 [details] Patch I see no objections. LGTM.
Comment on attachment 351595 [details] Patch Clearing flags on attachment: 351595 Committed r236993: <https://trac.webkit.org/changeset/236993>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45146994>