[lit][NFC] Cleanup lit worker process handling

[yln]

Summary

Move code that is executed on worker process to separate file. This
makes the use of the pickled arguments stored in global variables in the
worker a bit clearer. (Still not pretty though.)

Extract handling of parallelism groups to it's own function.

Use BoundedSemaphore instead of Semaphore. BoundedSemaphore raises for
unmatched release() calls.

Cleanup imports.

[rnk]

llvm/utils/lit/lit/run.py

196

Won't we get an unbound local error now if parallelism_semaphores[pg] raises KeyError? I would expect this to raise, hit the except block, then run the finally, where it would raise again.

199

I agree this check seems unnecessary. It seems like it would be a bug to insert None as a value in the parallelism_semaphores dict.

[yln]

llvm/utils/lit/lit/run.py

196

Yes, it raises, prints a stack trace, and crashes. (Instead of marking tests as unresolved when I misconfigure my parallelism groups.)

199

Your comment about None gave me an idea. I think it *would* be useful for intentionally stating "no restrictions" vs "unsure if configuration error" or just not concerned about parallelism. I will update the patch. Thanks!

[yln]

Widened scope of cleanup.

[yln]

@rnk
I just realized that we are still using "threads" for the display and command line options and different notions in the code: thread, jobs, workers. Since I am already at it, should I unify the terminology (in a separate patch)?

[rnk]

In D58196#1398802, @yln wrote:

@rnk
I just realized that we are still using "threads" for the display and command line options and different notions in the code: thread, jobs, workers. Since I am already at it, should I unify the terminology (in a separate patch)?

Sure. The way I see it "worker" is a more general term, so it's reasonable to use it in comments even if we use the --numThreads command line option.

llvm/utils/lit/lit/worker.py

50–52 ↗

(On Diff #186938)

This code is 2-space indented, but the rest of lit is mostly 4-space.

[yln]

Fix indentation.

[rnk]

lgtm

llvm/utils/lit/lit/run.py

def consume_test_result(self, pool_result):
         # If we've finished all the tests or too many tests have failed, notify
         # the main thread that we've stopped testing.
         self.failure_count += (test_with_result.result.code == lit.Test.FAIL)
         if self.lit_config.maxFailures and \
                 self.failure_count == self.lit_config.maxFailures:
             self.hit_max_failures = True
 
 def _execute_test_impl(test, lit_config, parallelism_semaphores):
-    """Execute one test"""
+    """Execute one test with the appropriate level of parallelism"""
     pg = test.config.parallelism_group
     if callable(pg):
         pg = pg(test)
 
-    result = None
-    semaphore = None
     try:
         if pg:
             semaphore = parallelism_semaphores[pg]
-        if semaphore:
             semaphore.acquire()
         start_time = time.time()
         result = test.config.test_format.execute(test, lit_config)
         # Support deprecated result from execute() which returned the result
         # code and additional output as a tuple.
         if isinstance(result, tuple):
             code, output = result
             result = lit.Test.Result(code, output)
         elif not isinstance(result, lit.Test.Result):
             raise ValueError("unexpected result from test execution")
         result.elapsed = time.time() - start_time
     except KeyboardInterrupt:
         raise
     except:
         if lit_config.debug:
             raise
         output = 'Exception during script execution:\n'
         output += traceback.format_exc()
         output += '\n'
         result = lit.Test.Result(lit.Test.UNRESOLVED, output)
     finally:
-        if semaphore:
+        if pg:
             semaphore.release()
 
     test.setResult(result)
 
 child_lit_config = None
 child_parallelism_semaphores = None
 
 def worker_initializer(lit_config, parallelism_semaphores):