Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#4522 enhancement closed fixed (fixed)

Speed up `reactor.spawnProcess()` by only closing open file descriptors

Reported by: Carlos Valiente Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: itamarst Branch: branches/faster-close-fds-4522
branch-diff, diff-cov, branch-cov, buildbot
Author: cvaliente

Description

I've noticed that reactor.spawnProcess() tries to close all file descriptors in the range [0, <value of ulimit -n>].

I've run the following program on my Ubuntu 9.10 i386 box:

from twisted.internet import protocol, reactor


def spawn():

    def processEnded(*args):
        reactor.stop()

    proto = protocol.ProcessProtocol()
    proto.processEnded = processEnded
    reactor.spawnProcess(proto, "/bin/true", ["/bin/true"])


if __name__ == "__main__":
    reactor.callWhenRunning(spawn)
    reactor.run()

strace(1) shows that it spent about 70 ms on closing unopened file descriptors:

19:16:29.770372 clone(Process 4715 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xb777d728) = 4715
[pid  4714] 19:16:29.770934 close(3)    = 0
...
[pid  4715] 19:16:29.780888 close(0)    = 0
[pid  4715] 19:16:29.780921 close(1)    = 0
[pid  4715] 19:16:29.780953 close(2)    = 0
[pid  4715] 19:16:29.780984 close(4)    = 0
[pid  4715] 19:16:29.781015 close(5)    = 0
[pid  4715] 19:16:29.781046 close(6)    = 0
[pid  4715] 19:16:29.781077 close(7)    = 0
[pid  4715] 19:16:29.781109 close(9)    = 0
[pid  4715] 19:16:29.781140 close(11)   = -1 EBADF (Bad file descriptor)
[pid  4715] 19:16:29.781223 close(12)   = -1 EBADF (Bad file descriptor)
[pid  4715] 19:16:29.781267 close(13)   = -1 EBADF (Bad file descriptor)
...
[pid  4715] 19:16:29.850504 close(1022) = -1 EBADF (Bad file descriptor)
[pid  4715] 19:16:29.850542 close(1023) = -1 EBADF (Bad file descriptor)
[pid  4715] 19:16:29.850607 dup2(3, 0)  = 0
...

Following James' advice from http://twistedmatrix.com/pipermail/twisted-python/2010-June/022451.html, I've written a patch that tries to gather the list of open file descriptors from /proc/<pid>/fd, and limits the calls to close() to those ones. With that patch, the output of strace(1) shows a better behaviour:

19:21:36.547234 clone(Process 4727 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xb77ba728) = 4727
[pid  4727] 19:21:36.548056 open("/proc/4727/fd", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC <unfinished ...>
[pid  4726] 19:21:36.548089 close(7)    = 0
[pid  4727] 19:21:36.555612 <... open resumed> ) = 13
[pid  4727] 19:21:36.555668 getdents64(13, /* 16 entries */, 32768) = 384
[pid  4727] 19:21:36.555918 getdents64(13, /* 0 entries */, 32768) = 0
[pid  4727] 19:21:36.555961 close(13)   = 0
[pid  4727] 19:21:36.556147 close(0)    = 0
[pid  4727] 19:21:36.556190 close(1)    = 0
[pid  4727] 19:21:36.556229 close(2)    = 0
[pid  4727] 19:21:36.556268 close(3)    = 0
[pid  4727] 19:21:36.556306 close(4)    = 0
[pid  4727] 19:21:36.556345 close(5)    = 0
[pid  4727] 19:21:36.556383 close(6)    = 0
[pid  4727] 19:21:36.556422 close(8)    = 0
[pid  4727] 19:21:36.556461 close(9)    = 0
[pid  4727] 19:21:36.556500 close(11)   = 0
[pid  4727] 19:21:36.556539 close(13)   = -1 EBADF (Bad file descriptor)
[pid  4727] 19:21:36.556723 dup2(7, 0)  = 0
...

Attachments (9)

0001-Speed-up-reactor.spawnProcess.patch (2.1 KB) - added by Carlos Valiente 12 years ago.
Generated from the Git mirror
0002-Changes-from-James-review.patch (4.4 KB) - added by Carlos Valiente 12 years ago.
Addressing James' review
0003-Fix-test-for-_listOpenFDs.patch (1.2 KB) - added by Carlos Valiente 12 years ago.
Generated using Git from branch 'svn/faster-close-fds-4522'
0004-Skip-test_listOpenFDs-on-non-POSIX-platforms.patch (1017 bytes) - added by Carlos Valiente 12 years ago.
Generated using Git from branch 'svn/faster-close-fds-4522'
0005-Explicit-test-for-_listOpenFDs.patch (1.6 KB) - added by Carlos Valiente 12 years ago.
Generated using Git from branch 'svn/faster-close-fds-4522'
0006-Address-review-from-exarkun.patch (2.5 KB) - added by Carlos Valiente 12 years ago.
Diff against svn://svn.twistedmatrix.com/svn/Twisted/branches/faster-close-fds-4522, revision 29522
0007-Address-review-from-exarkun.patch (917 bytes) - added by Carlos Valiente 12 years ago.
Diff against svn://svn.twistedmatrix.com/svn/Twisted/branches/faster-close-fds-4522, revision 29557
0008-Also-use-_listOpenFDs-in-PTYProcess.patch (4.4 KB) - added by Carlos Valiente 12 years ago.
Generated from Git mirror, branch svn/faster-close-fds-4522
0009-Test-PTYProcess.patch (2.1 KB) - added by Carlos Valiente 12 years ago.
Generated from Git mirror, branch svn/faster-close-fds-4522

Download all attachments as: .zip

Change History (36)

Changed 12 years ago by Carlos Valiente

Generated from the Git mirror

comment:1 Changed 12 years ago by Glyph

More portable (supported both on the linuxes I have looked at as well as BSDs and OS X) would be to use /dev/fd, which also avoids the need for a getpid call.

Does anyone know if that is covered by a standard of some kind?

comment:2 Changed 12 years ago by Itamar Turner-Trauring

Cc: itamarst added

Using /proc only speeds up Linux. A more cross-platform alternative, a C extension to do the closing, would probably be as fast, but still has the issue that it's hard to figure out the maximum fd number (I'd probably go with a max that's bigger than 1024 if we're going the C route).

If we do go with this patch, it's going to need a unit test, which probably requires moving the "figure out fds to close" into a separate function.

For the record, I personally only care about Linux performance, but many people do use OS X and FreeBSD, so ideally we'd speed up all POSIX platforms.

comment:3 in reply to:  2 Changed 12 years ago by Glyph

Replying to itamar:

Using /proc only speeds up Linux.

Yes, but pretty much every UNIX-like OS has some way to do this, and I'm pretty sure all the ones we support have a working /dev/fd.

A more cross-platform alternative, a C extension to do the closing, would probably be as fast, but still has the issue that it's hard to figure out the maximum fd number (I'd probably go with a max that's bigger than 1024 if we're going the C route).

This still makes a ton of syscalls, which could be almost as bad as a ton of Python function calls, depending on the platform.

If we do go with this patch, it's going to need a unit test, which probably requires moving the "figure out fds to close" into a separate function.

We may also want to have an option to just trust the close-on-exec flag and not even attempt to close file descriptors.

comment:4 in reply to:  1 Changed 12 years ago by Carlos Valiente

Replying to glyph:

Does anyone know if that is covered by a standard of some kind?

According to http://stackoverflow.com/questions/899038/getting-the-highest-allocated-file-descriptor, there does not seem to be a single standard way.

comment:5 Changed 12 years ago by jknight

It looks like we can cover everything that anybody cares about by checking both /proc/pid/fd and /dev/fd. So, update the patch to check both of them, make the determination of fds to close be a separate function, and add a unit test that ensures that it only returns fds that are actually open.

I would actually check both paths, even though on my linux box /dev/fd is a symlink to /proc/self/fd, because of restricted environments where the user may have /proc mounted but not a fully-populated /dev.

For a case test, probably it would be fine to spawn a subprocess that does: print listOpenFDs() (where listOpenFDs is the new function) and then assert that it returns only [0,1,2,3], and not, say, xrange(1024).

For any platform we come across in the future that doesn't have a way to do fd enumeration, we can just mark that test as knownfail.

comment:6 Changed 12 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: changed from Glyph to Carlos Valiente

Based on James' review, reassigning back to cvaliente.

Changed 12 years ago by Carlos Valiente

Addressing James' review

comment:7 Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

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

Author: exarkun
Branch: branches/faster-close-fds-4522

(In [29403]) Branching to 'faster-close-fds-4522'

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

It'd be nice if os.closerange were suitable here, but it doesn't let you specify exactly which fds to close, just low and high values of a range.

comment:10 Changed 12 years ago by jknight

Well, os.closerange could be useful, but only on 2.6+, and only on platforms for which the listdir approach doesn't work.

Calling os.closerange(fd, fd+1) is probably faster than os.close(fd) when the fd doesn't exist, because the prior ignores the error and the latter has to create an exception.

But given that all platforms implement /dev/fd or /proc/pid/fd, and that the use of closerange would have to be conditionalized on 2.6+, it's probably not really worth it.

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

Author: exarkuncvaliente, exarkun
Keywords: review removed
Owner: set to Carlos Valiente

There are a few trivial points:

  1. FilePath should be preferred over os.path
  2. assertEquals should be preferred over assertEqual
  3. The print in test_listOpenFDs should be removed or turned into a log message
  4. listChildFDs should probably be private, at least for now. This might be a nice API to expose publically eventually, with an implementation that works on Windows too. For now it's just an internal helper and we should discourage people from using it.
  5. generator expressions aren't supported on Python 2.4, so they can't be used in Twisted.
  6. top-level suites (like the implementations of listOpenFDs and the following Process class) should be separated by three blank lines.

I've fixed these in r29405. More significantly:

  1. test_listOpenFDs only applies to POSIX platforms. It should be skipped elsewhere.
  2. I think there should be some direct unit tests for listOpenFDs. This will be increasingly important if we have to add more platform-specific cases.

Changed 12 years ago by Carlos Valiente

Generated using Git from branch 'svn/faster-close-fds-4522'

Changed 12 years ago by Carlos Valiente

Generated using Git from branch 'svn/faster-close-fds-4522'

Changed 12 years ago by Carlos Valiente

Generated using Git from branch 'svn/faster-close-fds-4522'

comment:12 in reply to:  11 Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

Replying to exarkun:

There are a few trivial points: ..

Thanks for the cleanup, exarkun. I've fixed the test case for the spawned process in 0003-Fix-test-for-_listOpenFDs.patch

More significantly:

  1. test_listOpenFDs only applies to POSIX platforms. It should be skipped elsewhere.

Done in 0004-Skip-test_listOpenFDs-on-non-POSIX-platforms.patch.

  1. I think there should be some direct unit tests for listOpenFDs. This will be increasingly important if we have to add more platform-specific cases.

Done in 0005-Explicit-test-for-_listOpenFDs.patch. I try writing a zero-legth string on each file descriptor returned by _listOpenFDs(). I assume that either the write succeeds, beacuse the file descriptor is open, or the write fails with EBADF, because the file descriptor is closed.

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

Author: cvaliente, exarkuncvaliente
Keywords: review removed
Owner: set to Carlos Valiente

I've fixed the test case for the spawned process in 0003-Fix-test-for-_listOpenFDs.patch

Thanks. I had some trouble with that. :)

Done in 0004-Skip-test_listOpenFDs-on-non-POSIX-platforms.patch Done in 0005-Explicit-test-for-_listOpenFDs.patch .

These two got a little mixed up I think, so replying to both at once:

The approach for skipping in test_openFileDescriptors looks fine. You didn't apply it to test_listOpenFDs, but I don't think it's ideal there anyway. Methods on ProcessTestsBuilder (and more generally, subclasses of ReactorBuilder) get blown up into 8 tests (and maybe more in the future) and applied to each reactor implementation. listOpenFDs doesn't go anywhere near the reactor though, so it's both redundant to test it against multiple posix reactors and silly to skip testing it repeatedly for all of the non-posix reactors.

More, test_process.py is trying to be a set of blackbox tests for IReactorProcess, so a test for an implementation detail of the posix reactors doesn't really fit in this module. There seems to be no whitebox test module for twisted/internet/process.py, so I think we should create one and drop test_listOpenFDs there.

On IRC last night, James Knight and I were discussing what the direct unit tests might look like. We talked about a test basically like this:

    f = file(devnull)
    fileno = f.fileno()
    self.assertIn(fileno, listOpenFDs())
    f.close()
    self.assertNotIn(fileno, listOpenFDs())

I think your EBADF test is good as well, since it test things from the other direction (makes sure results are valid fds, whereas this makes sure that a valid fd is in the result). Perhaps both of these tests could go into a new twisted/internet/test/test_posixprocess.py.

Thanks again for working on this. :)

comment:14 Changed 12 years ago by Carlos Valiente

Thanks for the comments, exarkun. So, just to check if I understood correctly:

  • test_listOpenFDs() goes into the new twisted/internet/test/test_posixprocess.py, augmented with tests like the one you describe on your last comment, and skipped on non-POSIX platforms.
  • test_openFileDescriptors() also goes into the new twisted/internet/test/test_posixprocess.py, and skipped on non-POSIX platforms, too.

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

test_listOpenFDs() goes into the new twisted/internet/test/test_posixprocess.py

Yep.

test_openFileDescriptors() also goes into the new twisted/internet/test/test_posixprocess.py

I think test_openFileDescriptors can stay where it is. It actually tests a behavior of the reactor: that no unexpected file descriptors are passed to the child process.

comment:16 in reply to:  15 Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

test_listOpenFDs() goes into the new twisted/internet/test/test_posixprocess.py

Yep.

test_openFileDescriptors() also goes into the new twisted/internet/test/test_posixprocess.py

I think test_openFileDescriptors can stay where it is. It actually tests a behavior of the reactor: that no unexpected file descriptors are passed to the child process.

Done in 0006-Address-review-from-exarkun.patch.

For test_listOpenFDs I started with this:

f = file(os.devnull)
fileno = f.fileno()                        # Returns 4
self.assertIn(fileno, _listOpenFDs())      # OK:     4 in [0, 1, 2, 3, 4, 5, 6, 7]
f.close()
self.assertNotIn(fileno, _listOpenFDs())   # FAILS:  4 in [0, 1, 2, 3, 4, 5, 6]

I assume what happens is the following:

  • Before the test, file descriptors [0, 1, 2, 3, 5, 6] are open
  • file(os.devnull) is assigned file descriptor 4. The call to os.listdir("/dev/fd") within _listOpenFDs makes file descriptor 7 appear in the reported list.
  • When f is closed, file descriptor 4 becomes available. The following call to os.listdir("/dev/fd") within _listOpenFDs uses file descriptor 4.

Any thoughts on this?

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

Keywords: review removed
Owner: set to Carlos Valiente

When f is closed, file descriptor 4 becomes available. The following call to os.listdir("/dev/fd") within _listOpenFDs uses file descriptor 4.

Ah of course, that old story. You can force a hole in the fd space with os.dup. So something like...

f = file(os.devnull) # Lowest available fd allocated
fd = os.dup(f.fileno()) # Next lowest available fd allocated
try:
    f.close() # original lowest fd is now available again
    self.assertIn(fd, _listOpenFDs()) # fd should be here, and listdir() will use original lowest fd
finally:
    os.close(fd)
self.assertNotIn(fd, _listOpenFDs()) # fd should not be here, and listdir() will *still* use original lowest fd

It's a bit of a hack. Still, it should get the job done. :)

comment:18 in reply to:  17 Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

Ah of course, that old story. You can force a hole in the fd space with os.dup. So something like...

Done in 0007-Address-review-from-exarkun.patch.

Amazing how much one ends up learning just by contributing a basically trivial 10-line patch :-)

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

Keywords: review removed
Owner: set to Carlos Valiente

Cool. This looks really good now. :) There's just one more thing (which I should have caught sooner...), I think.

_listOpenFDs is now being used in Process._setupChild to decide which fds to close. But if usePTY=True is passed to spawnProcess, then PTYProcess is used instead of Process. And PTYProcess._setupChild has a similar (but different! awesome) loop in it:

        for fd in xrange(3, 256):
            try:
                os.close(fd)
            except:
                pass

It'd be great to fix this as well.

Changed 12 years ago by Carlos Valiente

Generated from Git mirror, branch svn/faster-close-fds-4522

comment:20 in reply to:  19 ; Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

Replying to exarkun:

if usePTY=True is passed to spawnProcess, then PTYProcess is used instead of Process. And PTYProcess._setupChild has a similar (but different! awesome) loop in it:

        for fd in xrange(3, 256):
            try:
                os.close(fd)
            except:
                pass

It'd be great to fix this as well.

Done in 0008-Also-use-_listOpenFDs-in-PTYProcess.patch. I also moved test_openFileDescriptors upstairs into ProcessTestsBuilderBase, so that it also gets executed on PTYProcess tests.

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

Keywords: review removed
Owner: set to Carlos Valiente

Good catch on moving the test up to ProcessTestsBuilderBase. But argh. There's a hitch. Just being defined on the base doesn't actually cause the test to be applied to both PTY and non-PTY processes. The test has to pass the value of self.usePTY for the usePTY argument to spawnProcess.

But the test is using getProcessOutput and getProcessOutput doesn't accept a usePTY parameter, or have any way other way to specify that the spawnProcess call it makes should be passed usePTY=True.

Adding a usePTY argument to getProcessOutput would be the most directly way to fix this, I suppose. But another way would be to stop using it in this test and instead call spawnProcess directly. That might not even be too hard, as you can probably re-use something like twisted.test.test_process.TrivialProcessProtocol to do all (fifteen or so lines of) the hard work.

Relatedly(-ish), there's pretty much no way to be sure that a Python child process won't spit out something on stderr, so tests should be resilient against that eventuality. Basically this just means completely ignoring stderr most of the time.

Aside from that, I also checked in a minor change to the way the skip in test_posixprocess is done, to avoid even importing the process module on Windows (which won't ever succeed).

Also, there seems to be some disagreement between various platforms about whether writing to a closed file descriptor should result in EBADF or EINVAL. You can see this in the results of some builds I triggered (if you aren't familiar with buildbot, click on one of the revision links for a failed (red) build, then follow the "problems" link and look at the end of the output). SUSv7 allows write to fail with EINVAL, but the reason given doesn't make any sense. After writing all that and thinking about it a bit, I decided to try out an alternate approach w/ fcntl. fcntl(fd, F_GETFL) seems to produce EBADF more reliably, so I've updated the branch to do that instead.

comment:22 in reply to:  20 Changed 12 years ago by Glyph

Replying to cvaliente:

Done in 0008-Also-use-_listOpenFDs-in-PTYProcess.patch. I also moved test_openFileDescriptors upstairs into ProcessTestsBuilderBase, so that it also gets executed on PTYProcess tests.

Just a few words of encouragement from the peanut gallery: thanks for sticking with this patch through so many iterations! Keep up the good work, I'm looking forward to seeing it land :).

Changed 12 years ago by Carlos Valiente

Attachment: 0009-Test-PTYProcess.patch added

Generated from Git mirror, branch svn/faster-close-fds-4522

comment:23 Changed 12 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

Thanks for the review (and the encouragemente :-)). In 0009-Test-PTYProcess.patch I'm now calling reactor.spawnProcess directly.

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

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

Almost there.

The last issue is that when you have a pty, your stdout and stderr are actually the same thing, so the pty version of this test still fails on one of the builders where importing twisted in the child process triggers a warning.

sigh.

Maybe the right thing to do is suppress all warnings in the child process for this test. I don't really like the idea, but considering the warnings are mostly useless noise, it's probably the best approach.

This is easy, fortunately. Just add -Wignore to the command line.

The rest looks good to me now. Assuming the latest builds succeed, I'll merge.

Thanks a lot!

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

Resolution: fixed
Status: assignedclosed

(In [29597]) Merge faster-close-fds-4522

Author: cvaliente Reviewer: exarkun Fixes: #4522

Speed up reactor.spawnProcess by trying to discover which file descriptors are open and only closing those, rather than iterating over an entire range of potentially valid descriptors and trying to close them all.

comment:26 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:27 Changed 11 years ago by Glyph

People interested in this ticket's history are likely also be interested in #4881, which catalogues its interaction with the BSD family.

Note: See TracTickets for help on using tickets.