1174262 - Turn the windows jstests task runner into a generator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: turn_windows_task_running_into_a_generator-v0.diff

One of the main things holding back jstests from being able to scan for tests and run them at the same time is the fact that tasks_win.py cannot easily be turned into a generator. The design, in its current form, is *very* object-oriented. There's nothing wrong with that, necessarily, but it does mean that the results structs get pushed to the results processor deep in the runner's internals, which is problematic for making the runner's owner function a generator. Also, it forces a ton of extra boilerplate in the form of munging data into classes that is mostly just extra typing here. And there's also a ton of extra unrelated boilerplate, like an entire custom logging infrastructure that I'd bet real money has never been used, even once.

The attached patch replaces all this with a single, simple generator function that fits on a single page and should be trivially grokable.

The try run is green and it works for me locally. I don't think we have anyone else that will stoop to using windows at the moment, so this is probably good enough.

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8621731 [details] [diff] [review]
turn_windows_task_running_into_a_generator-v0.diff

Review of attachment 8621731 [details] [diff] [review]:
-----------------------------------------------------------------

Really nice, thanks for the education in Python threading.

How does this behave when you Ctrl-C? I just tried q.get(block=True) on my linux64 machine with an empty queue, and Ctrl-C doesn't work at all. Which could be a problem on Windows, with its lack of job control.

::: js/src/tests/lib/tasks_win.py
@@ +10,5 @@
> +def run_all_tests_gen(tests, results, options):
> +    """
> +    Uses scatter-gather to a thread-pool to manage children.
> +    """
> +    qSend, qResults = Queue(), Queue()

Hm. I would expect either qSend and qReceive, or qTasks and qResults, not a mixture of the two.

I prefer the latter, since

  test = qSend.get(True)

looks funny.

@@ +15,4 @@
>  
> +    def do_work():
> +        while True:
> +            test = qSend.get(True)

Can you name the argument? For people like me who have never seen any of this stuff before.

  test = qSend.get(block=True)

@@ +15,5 @@
>  
> +    def do_work():
> +        while True:
> +            test = qSend.get(True)
> +            if test is Ellipsis:

Just ooc, why Ellipsis and not, say, None? I guess None is too easy to come by accidentally. And False compares true to too many things.

Though I'd be tempted to do

  class GameOver: pass

(or 'EndMarker')

Comment 2

[Terrence Cole [:terrence]]

Great feedback!

* I've added timeout=sys.maxint to queue.get, which works around the problem in python2.
* I've renamed the queues as suggested.
* I've replaced Ellipsis with EndMarker.
* And I split the callback out and passed the args via copy instead of closing over them -- should be clearer like this when I integrate the watchdog threads.

Result: test times go from 146s -> 119s. Nice!

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47ed04af6273

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47ed04af6273