mpv
mpv copied to clipboard
mpv-player
[RFC] subprocess-posix: use `clone` in Linux and `rfork_thread` in FreeBSD
This PR introduces platform-specific implementations, namely clone in Linux and rfork_thread in FreeBSD, to optimize subprocess creation. vfork semantics is used in these implementations: virtual memory is shared between the parent and the child, and the execution of the parent suspends until the child terminates. (However, vfork is not used because its behavior varies across platforms, and it's difficult to be used properly in C.) Besides lower overhead, this simplifies the result passing: no pipe is needed.
I feel like introducing such additional complex, platform-specific code is not worth it for a vague "lower overhead" in process creation (when is this a bottleneck??).
when is this a bottleneck??
I agree, we should evaluate whether it is needed using some test cases that show the actual difference. mpv, under normal operation, shouldn't spawn subprocesses, especially not ones that are performance critical.
I feel like introducing such additional complex, platform-specific code is not worth it for a vague "lower overhead" in process creation (when is this a bottleneck??).
Well, I don't think it's complex. clone and rfork_thread share most of the logic; the only difference is their API names. And most code line addition results from the move of spawn_process into spawn_process_inner and child_main; most code blocks are reused from current implementation. I've tried my best to keep it neat.
You can also have a look at the new handling of detach. Previously, it is handled using redundant code, and now it shares the same code path.
These APIs are also used by posix_spawn, which is used before 53094977. So you can think of this PR a restore of performance.
Agree with what's already said. I think there is no reason to maintain 2 additional platform-specific implementations for little benefit when the existing one works. Additionally from FreeBSD documentation: "The rfork_thread() function has been deprecated in favor of pthread_create(3)."
You can also have a look at the new handling of
detach. Previously, it is handled using redundant code, and now it shares the same code path.
That's a separate issue and does not require the new process creation functions.
Download the artifacts for this pull request:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Additionally from FreeBSD documentation: "The rfork_thread() function has been deprecated in favor of pthread_create(3)."
Not related IMO. The latter is for creating threads. The functionality used in the code cannot be achieved by pthread_create. rfork_thread is just a little wrapper of rfork, which is not deprecated. And the FreeBSD libc also uses it.
maintain 2 additional platform-specific implementations
I don't consider they as two separated platform-specific implementations as they only differ in one single line of code of calling the API.
As Linux has the de facto dominance, I think it's acceptable to optimize for it with little additional code complexity. You can consider the FreeBSD part as a side product.
A microbenchmark:
local utils = require 'mp.utils'
local start_time = mp.get_time()
for i = 1,1000 do
utils.subprocess_detached{
args = {"/bin/echo", "test"}
}
end
local end_time = mp.get_time()
print(end_time - start_time)
Tested on Linux 6.11.0: Current: 2.881334s This PR: 0.126008s
Tested on FreeBSD 13.4: Current: 5.720950s This PR: 0.457035s
I think the performance improvement is nontrivial.
I think the performance improvement is nontrivial.
It is non trivial, but, without commenting on the PR itself, on its own it doesn't necessarily carry a lot of weight.
The use cases of mpv where hundreds of processes per second are not enough but (many) thousands is just what the doctor ordered are probably not all that common.
You have a track record of good code, so that's a good start, but the couter argument is that it's currently (probably) good enough and known to work where needed, while new code does carry risks.
But if the reviewers find the new code great, then such a perf improvement is certainly nice.
I think the performance improvement is nontrivial.
It is non trivial, but, without commenting on the PR itself, on its own it doesn't necessarily carry a lot of weight.
The use cases of mpv where hundreds of processes per second are not enough but (many) thousands is just what the doctor ordered are probably not all that common.
You have a track record of good code, so that's a good start, but the couter argument is that it's currently (probably) good enough and known to work where needed, while new code does carry risks.
But if the reviewers find the new code great, then such a perf improvement is certainly nice.
Yeah, we can put this aside.
These APIs are also used by
posix_spawn, which is used before 5309497. So you can think of this PR a restore of performance.
Worth noting that the new POSIX supports this use case: https://www.austingroupbugs.net/view.php?id=1044
These APIs are also used by
posix_spawn, which is used before 5309497. So you can think of this PR a restore of performance.Worth noting that the new POSIX supports this use case: https://www.austingroupbugs.net/view.php?id=1044
Yes. But we still need to perform a double-fork in order to detach a child process. Is posix_spawn going to be async-signal-safe? If not, we still cannot use it after the first fork (or clone); rfork does not have this issue because RFSPAWN clears all signal handlers. We can actually use clone3 with CLONE_CLEAR_SIGHAND on Linux to achieve the similar; but clone3 is not wrapped by glibc and CLONE_CLEAR_SIGHAND requires Linux 5.5+.
BTW in fact we can use pidfd with poll in one separated thread to reap detached child processes in Linux, so that double-fork can be avoided. However, such an implementation will grow too complicated.
But in any case, I agree with the rest. Seems not worth it unless there's actual use cases where this matters.
Worth noting that the new POSIX supports this use case: https://www.austingroupbugs.net/view.php?id=1044
Yes. But we still need to perform a double-fork in order to detach a child process.
setsid already "detaches" the child process, or is mpv expecting some other semantic that setsid doesn't provide?
With setsid() you shouldn't need double fork. And with posix_spawn + POSIX_SPAWN_SETSID you shouldn't need fork at all.
Also, posix_spawn has an option to reset all signal-handlers to default (posix_spawnattr_{setflags,setsigdefault}), so you wouldn't need the reset all signals in a loop that we currently have either.
If POSIX_SPAWN_SETSID is available on targets we care about, then pursuing that seems like it'd be worthwhile.
setsidalready "detaches" the child process, or is mpv expecting some other semantic that setsid doesn't provide?
The problem is that we still need to reap the zombies if we do not double-fork.
FWIW for anyone who is still interested in subprocess creation stuff, you may be also interested in my another PR: https://github.com/ziglang/zig/pull/22368.
The problem is that we still need to reap the zombies if we do not double-fork.
Ah, yeah... the zombies. Looked around a bit but it doesn't seem like posix_spawn has any flags/option to solve this.
Maybe there's a better way to reap the child so we don't need to reparent it to pid 1? (Nothing that comes to mind though...)
Maybe there's a better way to reap the child so we don't need to reparent it to pid 1? (Nothing that comes to mind though...)
We can waitpid(-1, ...) in a separated thread. An extra thread always has overhead, though. No silver bullet.
BTW we cannot assume orphans are reparented to pid 1. It's implementation-defined. And systemd handles orphans in a different way. See https://stackoverflow.com/a/40424702
We can
waitpid(-1, ...)in a separated thread.
I thought about doing it via SIGCHLD handler. But in either case, that's probably not an option since we'd also need to consider libmpv, I think.
One other thing I thought was to collect the detached pids into a dynamic array and attempt to reap them every once in a while with WNOHANG.
Not sure where the reaping code should go. Would require the dynamic array to be global which has the usual threading issues too. So, yeah...