Use our own Thread struct instead of std::thread #8

puck · 2024-06-09T13:23:28Z

puck commented

2024-06-09 13:23:28 +00:00

We'd highly prefer using std::thread here; but this won't let us configure the stack size. macOS uses 512KiB size stacks for non-main threads, and musl defaults to 128k. While Nix configures a 64MiB size for the main thread, this doesn't propagate to the threads we launch here. It turns out, running the evaluator under an anemic stack of 0.5MiB has it overflow way too quickly. Hence, we have our own custom Thread struct.

Requires a bit of C++ cleanup, but seems to work; and allows more complicated evaluations to run on darwin(-arm64) reliably.

We'd highly prefer using `std::thread` here; but this won't let us configure the stack size. macOS uses 512KiB size stacks for non-main threads, and musl defaults to 128k. While Nix configures a 64MiB size for the main thread, this doesn't propagate to the threads we launch here. It turns out, running the evaluator under an anemic stack of 0.5MiB has it overflow way too quickly. Hence, we have our own custom Thread struct. Requires a bit of C++ cleanup, but seems to work; and allows more complicated evaluations to run on darwin(-arm64) reliably.

puck added 1 commit 2024-06-09 13:23:31 +00:00

Use our own Thread struct instead of std::thread 577ef6e478

We'd highly prefer using std::thread here; but this won't let us configure the stack
size. macOS uses 512KiB size stacks for non-main threads, and musl defaults to 128k.
While Nix configures a 64MiB size for the main thread, this doesn't propagate to the
threads we launch here. It turns out, running the evaluator under an anemic stack of
0.5MiB has it overflow way too quickly. Hence, we have our own custom Thread struct.

jade reviewed 2024-06-09 14:58:19 +00:00

src/nix-eval-jobs.cc Outdated

					
				@ -102,0 +104,4 @@

				// While Nix configures a 64MiB size for the main thread, this doesn't propagate to the

				// threads we launch here. It turns out, running the evaluator under an anemic stack of

				// 0.5MiB has it overflow way too quickly. Hence, we have our own custom Thread struct.

				struct Thread {

jade commented

2024-06-09 14:35:18 +00:00

maybe our brain is constantly having too much of a crab rave, but this particular struct feels very risky lifetimes wise: if you ever destroy the backing memory without joining the thread first, you blow up the closure of the std::function, unless they miraculously made std::function a shared pointer or something.

thinking out loud: it seems potentially desirable to put the entire structure into a shared_ptr that itself internally retains a reference to as long as the thread execution inside lives. and even that feels potentially like it's not strictly correct at destruction time from an orphaned thread.

orrr on thread init you just move the std function into a stack variable, but hmmm, you actually would want to hand it something that it only has to destruct from the outer scope, to be sound against the thread taking a while to create and it getting destroyed in the mean time. yeah ok. so you would change the context type you give the inner thread to just a unique_ptr of std::function, which you .release() when you create the thread, then reconstitute into a unique_ptr inside init().

that should be trivially sound, yeah.

an alternative you could do to be suitably annoying about this is to just join the thread in the destructor, which probably would have to be able to throw. oh no how does this deal with exceptions :(

join needs to probably do stuff with exception_ptr. ok so then there does have to be shared state, fiddlesticks. probably then instead of unique_ptr of std::function you just want a little inner struct with exception pointer and the function as before, and then the context argument to thread creation is a released unique_ptr of that shared_ptr that's then reconstituted by the same manner as above. that then ensures that one ref count is leaked until the thread actually starts and reclaims it.

maybe our brain is constantly having too much of a crab rave, but this particular struct feels very risky lifetimes wise: if you ever destroy the backing memory without joining the thread first, you blow up the closure of the std::function, unless they miraculously made std::function a shared pointer or something. thinking out loud: it seems potentially desirable to put the entire structure into a shared_ptr that itself internally retains a reference to as long as the thread execution inside lives. and even that feels potentially like it's not strictly correct at destruction time from an orphaned thread. orrr on thread init you just move the std function into a stack variable, but hmmm, you actually would want to *hand* it something that *it only* has to destruct from the outer scope, to be sound against the thread taking a while to create and it getting destroyed in the mean time. yeah ok. so you would change the context type you give the inner thread to *just* a unique_ptr of std::function, which you .release() when you create the thread, then reconstitute into a unique_ptr inside init(). that should be trivially sound, yeah. an alternative you could do to be suitably annoying about this is to just join the thread in the destructor, which probably would have to be able to throw. oh no how does this deal with exceptions :( join needs to probably do stuff with exception_ptr. ok so then there does have to be shared state, fiddlesticks. probably then instead of unique_ptr of std::function you just want a little inner struct with exception pointer and the function as before, and then the context argument to thread creation is a released unique_ptr of that shared_ptr that's then reconstituted by the same manner as above. that then ensures that one ref count is leaked until the thread actually starts and reclaims it.

puck commented

2024-06-12 22:41:40 +00:00

This should be mostly fixed now? Dropping the thread will just detach it; and joining it now behaves as intended.

puck force-pushed own-thread from 577ef6e478 to 11d467fecd

2024-06-12 22:40:50 +00:00

Compare

jade approved these changes 2024-06-13 21:42:46 +00:00

jade left a comment

looks sound to me

jade commented

2024-06-13 21:43:30 +00:00

it does lose exceptions (most likely just into exploding tho, which is ok), which is somewhat bothersome, but this is an improvement for now, i think.

puck changed title from ~~WIP: Use our own Thread struct instead of std::thread~~ to Use our own Thread struct instead of std::thread

2024-06-14 20:12:59 +00:00

puck merged commit 11d467fecd into main

2024-06-14 20:13:24 +00:00

Sign in to join this conversation.

No reviewers