Bug 63771 – Remove threaded mode from new-run-webkit-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove threaded mode from new-run-webkit-tests

[Adam Barth]

Created attachment 99399 [details]
Patch

[Dirk Pranke]

Comment on attachment 99399 [details]
Patch

This comment refers to the actual contents of the patch, not the wisdom of removing the feature ...

I believe there are tests in run_webkit_tests_integrationtest.py that depend on --worker-model=threads being valid. You should probably verify that test-webkitpy still passes on both python2.5 and python2.6.

> Tools/Scripts/webkitpy/common/system/executive.py:-133
> -                # http://bugs.python.org/issue1731717

Even if you are removing the current uses of threads in python, I would probably leave this and the following comment in, unless you have some way of barring people from ever using this code in threads in the future

(Given that these are rather surprising characteristics).

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:43
> +FIXME: There is no such thing as TestRunner2.

What was TestRunner2 is now known as the Manager. You can just update the comment.

[Eric Seidel (no email)]

Comment on attachment 99399 [details]
Patch

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

Maybe there are too many poll() comments.  I added them all when trying to track down the mac threading problems a year ago.  Bug if we're not using theading, they're not very useful.  Up to you.

>> Tools/Scripts/webkitpy/common/system/executive.py:-133
>> -                # http://bugs.python.org/issue1731717
> 
> Even if you are removing the current uses of threads in python, I would probably leave this and the following comment in, unless you have some way of barring people from ever using this code in threads in the future
> 
> (Given that these are rather surprising characteristics).

Agreed.

[Dirk Pranke]

As far as whether you should actually remove this code, here are the reasons for leaving it in:

1) As noted in the earlier comment, I believe that there are tests that rely on threading to work in order to test other features. We should ensure that in removing this functionality we do not reduce test coverage.

2) On platforms that do not have Python 2.6 or later, threading is the only way to get multiple workers to run in parallel. It is conceivable that there may be developers stuck on 2.5 or bots stuck on 2.5 that wish to be able to run in parallel.

(As an aside, the "bugginess" referred to in the existing code is bugginess in the python platform itself (which certainly will not be fixed in a 2.5.x release. The bugginess that leads to workers wedging has never manifested itself using my test harnesses (e.g., MockDRT), which leads me to believe there is some weird interaction with DumpRenderTree that contributes to the problem).

3) It is conceivable that there will be platforms in the future that wish to use threads instead of processes. Of course, the code could probalby be added back in in that situation.

4) You cannot usefully NRWT under the python debugger in multiprocess mode to debug workers, but threaded mode works as well as most multithreaded debuggers work. Most of the time when I have needed to debug something in NRWT I use the "inline" mode instead of the "threaded" mode.

Given the above, I am inclined to leave the code in, but perhaps I am too emotionally invested in having written this code in the first place.

[Adam Barth]

I understand that it's hard to let code go that you've invested a lot of effort into, but truth of the matter is that the investment that's paying off here is your investment in getting the multiprocess worker model working.  It's that investment that we're leveraging to be able to unfork run-webkit-tests.

[Adam Barth]

> 1) As noted in the earlier comment, I believe that there are tests that rely on threading to work in order to test other features. We should ensure that in removing this functionality we do not reduce test coverage.

The tests all pass with this patch, and I don't think I removed any tests other than ones that were testing the threading implementation specifically.

As for the Python debugger, this might be my ignorance in that I don't know how to use it, so I don't see the cost of removing it.

[Adam Barth]

Comment on attachment 99399 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/system/executive.py:-133
>>> -                # http://bugs.python.org/issue1731717
>> 
>> Even if you are removing the current uses of threads in python, I would probably leave this and the following comment in, unless you have some way of barring people from ever using this code in threads in the future
>> 
>> (Given that these are rather surprising characteristics).
> 
> Agreed.

I've added this instance fo the comment back (as well as the wait() comment lower in this file, but I've left the other ones out.  The comments are really distracting to have everywhere in the codebase.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:43
>> +FIXME: There is no such thing as TestRunner2.
> 
> What was TestRunner2 is now known as the Manager. You can just update the comment.

Fixed.

[Adam Barth]

Created attachment 99409 [details]
Patch for landing

[Dirk Pranke]

(In reply to comment #6)
> > 1) As noted in the earlier comment, I believe that there are tests that rely on threading to work in order to test other features. We should ensure that in removing this functionality we do not reduce test coverage.
> 
> The tests all pass with this patch, and I don't think I removed any tests other than ones that were testing the threading implementation specifically.
> 

Yeah, I actually tested the patch locally and saw that test-webkitpy continued to pass, so it looks fine. I think I may have been thinking of the test_run_order() tests.

[WebKit Review Bot]

Comment on attachment 99409 [details]
Patch for landing

Clearing flags on attachment: 99409

Committed r90191: <http://trac.webkit.org/changeset/90191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Chromium Windows bot has been ever since this patch was landed :(

http://build.webkit.org/waterfall?show=Chromium%20Win%20Release%20(Tests)

[Eric Seidel (no email)]

E:\google-windows-2\chromium-win-release-tests\build\Tools\Scripts\webkitpy\layout_tests\port\webkit.pyc: WARNING  Failed to open Skipped file: /mock/LayoutTests/platform/testwebkitport/Skipped
..........Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "c:\Python26\lib\multiprocessing\forking.py", line 341, in main
    prepare(preparation_data)
  File "c:\Python26\lib\multiprocessing\forking.py", line 450, in prepare
    file, path_name, etc = imp.find_module(main_name, dirs)
ImportError: No module named test-webkitpy