This is an archive of the discontinued LLVM Phabricator instance.

[lit][NFC] Cleanup lit worker process handling
ClosedPublic

Authored by yln on Feb 13 2019, 11:05 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

yln created this revision.Feb 13 2019, 11:05 AM
yln edited the summary of this revision. (Show Details)Feb 13 2019, 11:10 AM
yln added reviewers: dcoughlin, kubamracek, delcypher, rnk.
rnk added inline comments.Feb 13 2019, 11:27 AM
llvm/utils/lit/lit/run.py
196 ↗(On Diff #186702)

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 ↗(On Diff #186702)

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 marked 2 inline comments as done.Feb 13 2019, 7:24 PM
yln added inline comments.
llvm/utils/lit/lit/run.py
196 ↗(On Diff #186702)

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

199 ↗(On Diff #186702)

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 updated this revision to Diff 186938.Feb 14 2019, 4:05 PM
yln marked 2 inline comments as done.

Widened scope of cleanup.

yln retitled this revision from [lit][NFC] Small cleanup to [lit][NFC] Cleanup lit worker process handling.Feb 14 2019, 4:06 PM
yln edited the summary of this revision. (Show Details)
yln added a comment.Feb 14 2019, 4:39 PM

@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 added a comment.Feb 15 2019, 1:04 PM
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 updated this revision to Diff 187079.Feb 15 2019, 1:48 PM
yln edited the summary of this revision. (Show Details)

Fix indentation.

yln marked an inline comment as done.Feb 15 2019, 1:49 PM
rnk accepted this revision.Feb 15 2019, 2:18 PM

lgtm

This revision is now accepted and ready to land.Feb 15 2019, 2:18 PM
This revision was automatically updated to reflect the committed changes.