Bug 204154 – test262-runner: use NUMBER_OF_PROCESSORS and stop multiplying the number of cores.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

We have enabled running test262 on GTK and WPE bots on r252362
But it seems this test is not respecting the environment variable NUMBER_OF_PROCESSORS that is set on the bot environment to tell each worker how much parallelism it should use.
Instead it is getting the system number of cores (which on some cases is 256) and that causes issues on other containers running on the same system.

This script should use the environment variable NUMBER_OF_PROCESSORS if is set, like all the other scripts for running webkit-related tests do

[Carlos Alberto Lopez Perez]

Created attachment 383453 [details]
Patch

[Ross Kirsling]

Comment on attachment 383453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383453&action=review

This basically seems good, but are we concerned about preserving the fact that the number of *processes* returned here is a multiple of the number of *processors*? (Honest question, since I was surprised to see that this is how it current works.)

> Tools/Scripts/test262/Runner.pm:556
> +

style nit: this initial newline could be removed

[Carlos Alberto Lopez Perez]

(In reply to Ross Kirsling from comment #2)
> Comment on attachment 383453 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383453&action=review
> 
> This basically seems good, but are we concerned about preserving the fact
> that the number of *processes* returned here is a multiple of the number of
> *processors*? (Honest question, since I was surprised to see that this is
> how it current works.)
> 

I also got surprised by that, and I have no idea why is it.

This multiplier was originally set to 8 when the script was added in r230680 and then modified depending on the number of cores in r231525 but none of the commits or bugs associated provide any info about why is that.

In any case I think that the environment variable NUMBER_OF_PROCESSORS=n should have the same effect that passing the parameter --child-processes=n or -p=n

And that is what this patch does.

[Ross Kirsling]

Hmm, I just tried doing a full release run with different child process counts on an 8-core iMac and saw the following:

8 processes:
> Done in 485.89 seconds!
> Tools/Scripts/test262-runner --release  1024.56s user 1517.22s system 522% cpu 8:06.06 total

16 processes:
> Done in 476.87 seconds!
> Tools/Scripts/test262-runner --release  1023.00s user 1525.18s system 534% cpu 7:57.10 total

32 processes:
> Done in 468.89 seconds!
> Tools/Scripts/test262-runner --release  1024.17s user 1536.59s system 545% cpu 7:49.11 total

So the total time is hardly affected, but my ability to interact with other applications on the same system is hugely affected (i.e. it's very hard just to browse webpages while the 32-process run is active).

Given this and a lack of written motivation for the existing behavior, perhaps we should remove the multiples even from the normal path here.

[Carlos Alberto Lopez Perez]

(In reply to Ross Kirsling from comment #4)
> Hmm, I just tried doing a full release run with different child process
> counts on an 8-core iMac and saw the following:
> 
> 8 processes:
> > Done in 485.89 seconds!
> > Tools/Scripts/test262-runner --release  1024.56s user 1517.22s system 522% cpu 8:06.06 total
> 
> 16 processes:
> > Done in 476.87 seconds!
> > Tools/Scripts/test262-runner --release  1023.00s user 1525.18s system 534% cpu 7:57.10 total
> 
> 32 processes:
> > Done in 468.89 seconds!
> > Tools/Scripts/test262-runner --release  1024.17s user 1536.59s system 545% cpu 7:49.11 total
> 
> So the total time is hardly affected, but my ability to interact with other
> applications on the same system is hugely affected (i.e. it's very hard just
> to browse webpages while the 32-process run is active).
> 
> Given this and a lack of written motivation for the existing behavior,
> perhaps we should remove the multiples even from the normal path here.

It looks reasonable. I will upload a new patch removing that multiplication.

[Carlos Alberto Lopez Perez]

Created attachment 383506 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 383506 [details]
Patch

Clearing flags on attachment: 383506

Committed r252437: <https://trac.webkit.org/changeset/252437>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57172650>

[Carlos Alberto Lopez Perez]

Another confirmation that the previous behaviour of multiplying cores didn't improved performance:


Before r252437, the Mac Test262 bots where taking to complete the step the following:

Release - 19:12 mins - Child Processes: 48 -  https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/7127
Debug - 20:11 mins - Child Processes: 32 - https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/6218

And after r252437 they take even less time to complete

Release - 18:28 mins - Child Processes: 24 - https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/7128
Debug - 19:40 mins - Child Processes: 16 - https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/6219

[Ross Kirsling]

Excellent!