Bug 185918 - test262/Runner.pm: randomize tests for performance
Summary: test262/Runner.pm: randomize tests for performance
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-23 12:08 PDT by valerie
Modified: 2018-05-29 11:36 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.27 KB, patch)
2018-05-23 12:24 PDT, valerie
valerie: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description valerie 2018-05-23 12:08:35 PDT
If there are more than 5*max_process number of test, then shuffle the tests before allocating to child process to split up groups of small tests.
Comment 1 valerie 2018-05-23 12:24:02 PDT
Created attachment 341115 [details]
Patch
Comment 2 Alexey Proskuryakov 2018-05-23 14:45:28 PDT
Comment on attachment 341115 [details]
Patch

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

> Tools/Scripts/test262/Runner.pm:296
> +        @files = shuffle @files;

Anything random in tests makes me nervous because it can make reproducing results hard. Can the same result be achieved without randomness?
Comment 3 valerie 2018-05-23 15:17:55 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 341115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341115&action=review
> 
> > Tools/Scripts/test262/Runner.pm:296
> > +        @files = shuffle @files;
> 
> Anything random in tests makes me nervous because it can make reproducing
> results hard. Can the same result be achieved without randomness?

Hmm -- so this randomize the order in which the tests are run to even the load across the child processes (there are some groups of slow tests). The tests all stand alone.

I thought of alphabetizing the tests (instead of shuffle), as the "slow" groupings are groupings within directories, but unfortunately the naming convention is also consistent in such a way that slow tests would be grouped.

This is not a very great performance increase -- and I'm not married to seeing it committed, it's just here to get feedback from Michael :)
Comment 4 Darin Adler 2018-05-28 20:36:31 PDT
I think that using randomness is OK to achieve this. But it just needs to be one-time randomness with the results stored in the repository, rather than "every time the tests are run" randomness.

Choosing a different set every time we run tests can make problems with one test affecting another into hard-to-reproduce flakiness. And we’d like to avoid that even if it’s unlikely.
Comment 5 Leo Balter 2018-05-29 11:36:23 PDT
Darin, I'm taking over this patch work as Valerie is in a vacation time this week.

I'm not show how we randomize and keep it for consecutive runs. The results are sorted anyway.

> tests can make problems with one test affecting another into hard-to-reproduce flakiness

Every test run is isolated from each other. The only thing that could be a problem in this case is memory usage from each call to JSC (for each test). This not prevented from the current alternative - tests order as it is by their file path - anyway.

> And we’d like to avoid that even if it’s unlikely.

The only guaranteed way to prevent any conflict here is to run the tests in a single process queue. That's not what being solved here, neither this patch will compromise it. Running in a single thread makes the run way slower but gives you a save ride to prevent shared memory usage from multiple JSC calls.

You should also consider Test262 has only unit tests - as a maintainer I can guarantee that - and we don't run anything such as stress tests or anything relying on performance.