Opened 12 years ago

Last modified 11 years ago

#2967 defect new

Reaping child processes has superlinear complexity on POSIX

Reported by: lukatmyshu Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: spiv, Jean-Paul Calderone Branch: branches/waitpid-2967
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description (last modified by Jean-Paul Calderone)

Currently, twisted (process.py) does one waitpid for every process that it has spawned. If Twisted has a few thousand plus child processes alive still this can cause a huge slowdown (after a few hours twisted was spending 90% of its time doing waitpid).

One possible solution to this problem is to call waitpid(-1) just once to check if there are any processes that can be reaped. According to 'man waitpid', this should be called in a loop until there are no more child processes to be reaped. We're currently using a similar modification on our servers, and twisted rarely consumes more than 2% of the CPU now (previously was upwards of 20% .... would require restarts every 7-8 hours).

A problem with this solution is that it could interfere with non-Twisted process creation APIs. If waitpid(-1) reaps a process someone else created, there's nothing useful Twisted can do with that information and the other process creation API will never be able to learn about its process exiting.

Another possible solution is to use the SA_SIGINFO flag to sigaction to install a signal handler which will be informed of the PID of child which exited.

Another possibility is to use waitid(-1) with the WNOWAIT flag to learn the exited process's pid efficiently without interfering with the exit status of the process. A caveat with this approach is that if you ever see a PID you weren't expecting come back from waitid(-1), you have to fall back to calling waitpid() for each PID which Twisted knows about which might have exited.

Another possible solution is for Twisted to create one child process it controls. Whenever an attempt is made to spawn a child process, that existing child process is used to create the new child process. Since Twisted is in complete control of that intermediate process, no unknown processes can be created, so waitpid(-1) is acceptable.

Attachments (1)

twisted-process-waitpid.patch (1.3 KB) - added by lukatmyshu 12 years ago.
Patch to process.py to have it call waitpid(-1) instead of waitpid(<pid>) on every process.

Download all attachments as: .zip

Change History (13)

Changed 12 years ago by lukatmyshu

Patch to process.py to have it call waitpid(-1) instead of waitpid(<pid>) on every process.

comment:1 Changed 12 years ago by Jean-Paul Calderone

Owner: changed from Glyph to lukatmyshu

This is a good change to make.

The patch has some problems, though:

  • It uses simple suites, which are forbidden by the coding standard.
  • It does exception handling for waitpid too broadly
  • It uses the "old" style of log.msg/log.err instead of using log.err with the new _why parameter.
  • It probably breaks the case where multiple processes are ready to be reaped, since it will only waitpid() successfully once.

Additionally, it's almost certainly the case that some behavior of this code is not properly unit tested (I didn't try applying the patch and running the tests, but since the patch seems to be broken wrt multiple zombie processes, if no existing unit test fails with it applied then there is definitely some missing coverage). Can you correct the above issues and add some unit tests?

comment:2 Changed 12 years ago by Jean-Paul Calderone

Oh, I forgot, the patch also leaves dead code, _BaseProcess.reapProcess, which should be deleted.

comment:3 Changed 12 years ago by therve

author: therve
Branch: branches/waitpid-2967

(In [22689]) Branching to 'waitpid-2967'

comment:4 Changed 12 years ago by therve

(In [22690]) Use waitpid(-1), but loop until failure, and add a test.

Refs #2967

comment:5 Changed 12 years ago by therve

Owner: changed from lukatmyshu to therve

comment:6 Changed 12 years ago by spiv

Cc: spiv added

I haven't looked at the patch closely, but I am in favour of making this change.

Note though that using waitpid(-1) has a theoretical disadvantage to the current code: it may reap processes other than those spawned by reactor.spawnProcess (e.g. if someone is using a library that also happens to create child processes).

This is probably an acceptable tradeoff (I doubt anyone actually depends on the existing behaviour, but who knows...), but it is a potentially significant change in behaviour. So it probably deserves an explicit comment to that effect in the code, and ideally also a mention in the NEWS file whenever we next make a release.

comment:7 Changed 12 years ago by Glyph

Owner: changed from therve to Glyph
Status: newassigned

comment:8 Changed 12 years ago by Glyph

As spiv said, we can't use waitpid(-1) to optimize this at the same time as supporting popen() Currently we don't really support using popen() but it's a very long-standing bug to fix this (#733); users trip over it all the time.

I do have an idea for improving performance a bit over the current strategy. The really pessimal case right now is if you spawn a zillion long-running processes, then repeatedly start and stop a few short-running processes. We could optimize this if we didn't have to actually reap all the processes all the time. The implementation strategy would have a 'reaper' object that would maintain a list of processes which it would partially iterate (using task.coiterate, say) each time a SIGCHLD was received. To deal most effectively with the pessimal case, we could sort this list by process lifetime and assume that shorter-lived processes were more likely to be reaped first. Also, by processing the list more slowly, if a large group of processes exited simultaneously, we could maintain a counter of exited processes and potentially reap more than one on a single pass through the list.

Another idea for optimizing this without breaking popen() would be to spawn a single dedicated process-spawning process which the reactor would internally issue messages to spawn processes (say, over a custom AMP protocol). The main process could pass file descriptors to sub-sub-processes via sendmsg() and SCM_RIGHTS (see man 7 unix if you are unfamiliar with this technique). That process would not be running application code and would therefore be able to rely on SIGCHLD and waitpid(-1) doing what we expect. This spawnProcess implementation could be implemented in terms of the current one, though; the reactor would run the process-spawner-process with the regular waitpid(pid)-ing spawnProcess API.

comment:9 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Description: modified (diff)
Summary: Patch to fix performance issue when twisted has spawned many processesReaping child processes has superlinear complexity on POSIX

The proposed fix may not be the correct one. There is a valid problem here, though. Adjusting the summary and description to reflect this.

comment:10 Changed 11 years ago by Jean-Paul Calderone

Description: modified (diff)

The problem with the SIGINFO solution in the description is that signals might get coalesced. That means you won't always know all the PIDs for processes which have exited, so you still have to fall back to waitpid(-1) or waitid(-1).

For what it's worth, here's a program which demonstrates signal coalescing, even when SA_NODEFER is being used:

#include <signal.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>

void child_sigaction(int signal, siginfo_t* info, void* context) {
        printf("%d %d\n", signal, info->si_pid);
}

int main(int argc, char** argv) {
        int i;
        struct sigaction child = {0};
        FILE* fd[3];

        child.sa_sigaction = child_sigaction;
        child.sa_flags = SA_RESTART | SA_NODEFER | SA_SIGINFO;

        if (sigaction(SIGCHLD, &child, NULL) < 0) {
                perror("sigaction");
        }

        printf("%d\n", getpid());
        fd[0] = popen("sleep 1", "r");
        fd[1] = popen("sleep 2", "r");
        fd[2] = popen("sleep 3", "r");

        raise(SIGSTOP);

        for (i = 0; i < 4; ++i) {
                sleep(2);
        }

        return 0;
}

If signals are not coalesced, running the programming, waiting at least 3 seconds, then foregrounding the program would result in 3 lines worth of signal/pid information being written to stdout. Instead, only one line is written.

(also editing the descriptor to add another possible solution)

comment:11 Changed 11 years ago by Glyph

Owner: Glyph deleted
Status: assignednew

comment:12 Changed 9 years ago by <automation>

Note: See TracTickets for help on using tickets.