Bug 190190 – Allow run-javascriptcore-tests to distribute tests between devices

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Guillaume Emont]

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.

[Guillaume Emont]

Created attachment 351511 [details]
Patch

WIP first version of the patch, to try on the EWS.

[Dominik Inführ]

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]

Created attachment 351525 [details]
Patch

WIP new version using a Struct and fixing missing spaces

[Dominik Inführ]

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]

Created attachment 351595 [details]
Patch

WIP now without unneeded .to_s

[Guillaume Emont]

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 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 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]

(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]

Ideally a JSC reviewer would review this one.

[Guillaume Emont]

Ping reviewer.

[Michael Catanzaro]

Comment on attachment 351595 [details]
Patch

I see no objections. LGTM.

[WebKit Commit Bot]

Comment on attachment 351595 [details]
Patch

Clearing flags on attachment: 351595

Committed r236993: <https://trac.webkit.org/changeset/236993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45146994>