WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.29 KB, patch)
2018-10-03 08:51 PDT
,
Guillaume Emont
no flags
Details
Formatted Diff
Diff
Patch
(11.25 KB, patch)
2018-10-04 06:27 PDT
,
Guillaume Emont
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/45146994
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug