Summary: | test262/Runner.pm: randomize tests for performance | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | valerie <valerie> | ||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | ap, commit-queue, darin, ews-watchlist, leo, msaboff, valerie, webkit-bug-importer, webkit-unassigned, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
valerie
2018-05-23 12:08:35 PDT
Created attachment 341115 [details]
Patch
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? (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 :) 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. 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. Comment on attachment 341115 [details]
Patch
Putting r- based on Darin's comment about one-time randomness.
|