Bug 141161 - Remove slow test unused code from run-jsc-stress-tests
Summary: Remove slow test unused code from run-jsc-stress-tests
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-02 08:23 PST by Csaba Osztrogonác
Modified: 2015-02-09 06:09 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.42 KB, patch)
2015-02-02 08:26 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2015-02-02 08:27 PST, Csaba Osztrogonác
fpizlo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-02-02 08:23:27 PST
Handling slow tests mechanism was added by http://trac.webkit.org/changeset/157887
16 months before to prepend the only one slow test to the run list instead of append.

LayoutTests/js/regress/script-tests/stepanov_container.js was removed from the trunk
13 months before - http://trac.webkit.org/changeset/162181

This hacky slow test mechanism wasn't used at all after it.
I think we don't need it at all, let's clean up the code.
Comment 1 Csaba Osztrogonác 2015-02-02 08:26:05 PST
Created attachment 245878 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-02-02 08:26:33 PST
Comment on attachment 245878 [details]
Patch

wrong patch
Comment 3 Csaba Osztrogonác 2015-02-02 08:27:30 PST
Created attachment 245879 [details]
Patch
Comment 4 Csaba Osztrogonác 2015-02-07 23:05:34 PST
ping?
Comment 5 Filip Pizlo 2015-02-08 06:51:24 PST
Comment on attachment 245879 [details]
Patch

Please don't remove this. :-). We use the --extra-tests flag to pass tests that cannot be made public for whatever reasons, and most of those use the slow mechanism. 

Plus I'm surprised that you think this is a hack. Of course you want slow-running tests to get pretended rather than appended, to aid with load balancing.
Comment 6 Csaba Osztrogonác 2015-02-09 02:41:22 PST
(In reply to comment #5)
> Comment on attachment 245879 [details]
> Patch
> 
> Please don't remove this. :-). We use the --extra-tests flag to pass tests
> that cannot be made public for whatever reasons, and most of those use the
> slow mechanism. 

OK, now I know it is used, I don't want to remove a used feature. But as
I mentioned in the description, r157887 added it for only one test which
was removed by r162181 and haven't found any reference if it is used. If
a feature has only non-public use cases, it would be great to have some
comment why is it needed in the code or in the bug report which added it.

> Plus I'm surprised that you think this is a hack. Of course you want
> slow-running tests to get pretended rather than appended, to aid with load
> balancing.

I still think and consider this code a hack. I understad 
the goal, but I'm surprised if it worked ever. 

Let's see the GNU make based runner:
all: test_done_0 test_done_1 test_done_2 test_done_3 ...
...

The order of the prerequisite doesn't guaranatee anything. If you 
want to make sure if test_done_0 runs before test_done_1, you have 
to add normal or order only prerequisite.

Let's see the shell based runner. It collects the tests with external
find call without any ordering and runner threads always pick up a
test from the end of the list to run. I checked this list file, it
is absolutely random on Linux and it is more or less random on OSX too:
./test_script_0
./test_script_1
./test_script_10
./test_script_100
./test_script_1000
./test_script_10000
./test_script_10001
...
./test_script_10009
./test_script_1001
...
./test_script_16998
./test_script_16999
./test_script_17
./test_script_170
./test_script_1700
./test_script_17000
...
./test_script_17025
./test_script_17026
./test_script_1703
./test_script_1704
...
./test_script_1709
./test_script_171
...

It seems as if it was sorted with sort, but not with numberic sort at all.
Comment 7 Csaba Osztrogonác 2015-02-09 02:42:34 PST
Otherwise close as resolved/invalid, because it is still used.
Comment 8 Csaba Osztrogonác 2015-02-09 03:09:28 PST
I fixed the shell test runner for you in bug141383. 

The make runner runs the tests in good order now 
on Linux and OSX too, but it isn't guaranteed at 
all, it is only a luck.
Comment 9 Filip Pizlo 2015-02-09 06:09:22 PST
(In reply to comment #8)
> I fixed the shell test runner for you in bug141383. 
> 
> The make runner runs the tests in good order now 
> on Linux and OSX too, but it isn't guaranteed at 
> all, it is only a luck.

Nope, not luck. GNU make has a predictable search that drives execution order. It's done it the same way for at least 15 years. I haven't seen it documented but they probably can't change it because it would break makefiles that accidentally depend on it.

We don't care about the parallel test runner achieving great scalability on other make implementations. 

Also, all we need here is luck. Any test can be run in any order, but we scale better if the slowest tests are fired off first. So if the make we ran on decided to break with tradition and use a different ordering then we would just lose the efficacy of the slow test optimization but this wouldn't affect the validity of the test run.