Add support for async worker processes

[mikekap]

The idea is that you might be able to parallelize internally in the worker process, with the benefit of shared memory to cache details about dependencies. Async worker processes can receive & run multiple commands concurrently. They respond with results asynchronously in any order.

PR Notes:

• The DSL arg is solo_async=True because async is a python keyword.
• I'm sort of lukewarm on the idea of ListenableFutures - open to suggestions. The only real benefit is that it makes testing concurrent execution/cancellation easier.
• In theory the Sync worker pool can be cleaned up with the new interface (less of a need for a BurrowedWorkerProcess & lifecycle). I did not do this to keep this PR manageable.

[v-jizhang]

@mikekap Please fix this test error: Check license of: src/com/facebook/buck/worker/WorkerProcessPool.java.
Thanks

[mikekap]

@v-jizhang fixed the missing license issue.

[v-jizhang]

@mikekap I discussed this PR with the Buck team and really for this huge change, it's hard for me to get it reviewed.
Here are some questions and concerns from the buck team:
Is this modularized (when without solo_async option, Buck never go through the code this person added)?
Does this change really improve the performance and how much?
And Please open an issue and discuss with us before dropping a massive PR to the repo.

Thanks

[mikekap]

Let me try to answer these. In terms of reviewing, it looks more daunting than it is because github doesn't have rename detection - WorkerProcessPoolSync is a renamed version of WorkerProcessPool with a 10 line method. The main logic changes that are here are in WorkerProcess. A lot of the other files are plumbing through a new boolean.

Is this modularized (when without solo_async option, Buck never go through the code this person added)?
This PR changes the way WorkerProcess works internally so not entirely. Specifically - there is a thread to read the output of the worker process as opposed to assuming the output will be returned synchronously. There's some machinery to deal with the work of matching the worker response to the request and completing futures.

As built, WorkerProcess doesn't know if it's used with async at all - it's just a question of whether multiple jobs are submitted concurrently or not.

Beyond that - no code is shared between sync/async worker processes.

Does this change really improve the performance and how much?

For us, it improves performance significantly. We compile Clojure via a JVM-based process. Clojure is Java based, but uses a lot of static initializers even after compilation, which makes dependency loading during compilation a bottleneck. This PR lets us use a single process to cache dependencies that we have already loaded and parallelize via regular futures in the worker process.

And Please open an issue and discuss with us before dropping a massive PR to the repo.

Sorry, my intent wasn't to burden. Apologies. We are running this in our CI - so it helps in our use case. Unfortunately my previous work around adding persistent processes didn't yield as much gains for our use case. I'm happy to restructure this as needed - not tied to the current implementation at all. I'm hoping to get this upstreamed so various genrule changes and the like don't require constant upkeep. Would you like me to file an issue to get the conversation started or should we continue the thread here?

[v-jizhang]

Let me try to answer these. In terms of reviewing, it looks more daunting than it is because github doesn't have rename detection - WorkerProcessPoolSync is a renamed version of WorkerProcessPool with a 10 line method. The main logic changes that are here are in WorkerProcess. A lot of the other files are plumbing through a new boolean.

Is this modularized (when without solo_async option, Buck never go through the code this person added)?
This PR changes the way WorkerProcess works internally so not entirely. Specifically - there is a thread to read the output of the worker process as opposed to assuming the output will be returned synchronously. There's some machinery to deal with the work of matching the worker response to the request and completing futures.

As built, WorkerProcess doesn't know if it's used with async at all - it's just a question of whether multiple jobs are submitted concurrently or not.

Beyond that - no code is shared between sync/async worker processes.

Does this change really improve the performance and how much?

For us, it improves performance significantly. We compile Clojure via a JVM-based process. Clojure is Java based, but uses a lot of static initializers even after compilation, which makes dependency loading during compilation a bottleneck. This PR lets us use a single process to cache dependencies that we have already loaded and parallelize via regular futures in the worker process.

And Please open an issue and discuss with us before dropping a massive PR to the repo.

Sorry, my intent wasn't to burden. Apologies. We are running this in our CI - so it helps in our use case. Unfortunately my previous work around adding persistent processes didn't yield as much gains for our use case. I'm happy to restructure this as needed - not tied to the current implementation at all. I'm hoping to get this upstreamed so various genrule changes and the like don't require constant upkeep. Would you like me to file an issue to get the conversation started or should we continue the thread here?

No apologies, my previous comment is just some thoughts from buck team.
No need to file an issue, we can post any conversations in this PR and I'll get this reviewed. Thank you very much.

[v-jizhang]

The variables readerShutdownSignal and watchdogShutdownSignal are used as flags, should we make them volatile boolean? That way we will have 2 less objects.

[mikekap]

Not quite - the latches are .await-able. Realistically these objects probably aren't memory-intensive for buck - there's a WorkerProcess object per long-lived process. You would rarely have more than ncpus of these processes running -- though I guess it depends how many worker tools you use!

Update: I got rid of these by killing the watchdog and went with your suggestion.

[v-jizhang]

Thanks you

[v-jizhang]

Why notifyAll? we have only one thread in waiting.

[v-jizhang]

This method is called in thread readerLoop, a different thread from submitJob, and both threads access commandExitCodes. Should we protect it?

[mikekap]

commandExitCodes is a ConcurrentHashMap so there's no need. Though honestly the threading here is a bit complex because of the watchdog. I updated the implementation to get rid of the watchdog (it was mostly for paranoia) and add a volatile boolean for the shutdown signal as you suggested. Also added a test for worker death.

The locking is still a bit more complex than I'd prefer, so I'm open to suggestions here - there's a weird internal vs external locking of data - externally this is used to lock submitJob & close, but the reader thread uses readerThread to be woken up when there is something to do.

[v-jizhang]

I think that's Ok.

[v-jizhang]

Once readerAlive becomes false, it stays false forever. Should it be set to true here?

[v-jizhang]

I'm confused with the readerAlive logic. Could you explain?

[mikekap]

Yep that's the intent. readerAlive is set to false only if the worker returns a malformed response. In this context, malformed responses are signs something terrible happened - even build step failures are well formed responses. This could only happen on e.g. malformed json or if the process just died. In that case, we stop attempting to communicate with the process at all and just fail all incoming requests until the build eventually close()s the process.

[v-jizhang]

I c, thank you.

[v-jizhang]

This can't be false, why need we check it?

[mikekap]

I think as written, this is definitely true. This is mostly guarding to future changes in the code - if e.g. the caller is synchronized by accident.

[v-jizhang]

lgtm. Thanks