Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

popen() slow on AIX due to large FOPEN_MAX value #44295

Closed
johnlallen mannequin opened this issue Dec 1, 2006 · 4 comments
Closed

popen() slow on AIX due to large FOPEN_MAX value #44295

johnlallen mannequin opened this issue Dec 1, 2006 · 4 comments
Labels
stdlib Python modules in the Lib dir

Comments

@johnlallen
Copy link
Mannequin

johnlallen mannequin commented Dec 1, 2006

BPO 1607087
Nosy @birkenfeld
Files
  • f_closem.patch: AIX F_CLOSEM popen patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2008-02-23.22:09:40.274>
    created_at = <Date 2006-12-01.20:05:34.000>
    labels = ['library']
    title = 'popen() slow on AIX due to large FOPEN_MAX value'
    updated_at = <Date 2008-02-23.22:09:40.273>
    user = 'https://bugs.python.org/johnlallen'

    bugs.python.org fields:

    activity = <Date 2008-02-23.22:09:40.273>
    actor = 'georg.brandl'
    assignee = 'none'
    closed = True
    closed_date = <Date 2008-02-23.22:09:40.274>
    closer = 'georg.brandl'
    components = ['Library (Lib)']
    creation = <Date 2006-12-01.20:05:34.000>
    creator = 'johnlallen'
    dependencies = []
    files = ['7630']
    hgrepos = []
    issue_num = 1607087
    keywords = ['patch']
    message_count = 4.0
    messages = ['51421', '51422', '62820', '62821']
    nosy_count = 4.0
    nosy_names = ['georg.brandl', 'hvbargen', 'johnlallen', 'schmir']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1607087'
    versions = ['Python 2.5']

    @johnlallen
    Copy link
    Mannequin Author

    johnlallen mannequin commented Dec 1, 2006

    Experimentation revealed that the os.popen234 family of methods was at least 10 times slower on AIX than it was on Solaris. It turns out that this is because of the _run_child method in popen2.py, which has this definition:

        def _run_child(self, cmd):
            if isinstance(cmd, basestring):
                cmd = ['/bin/sh', '-c', cmd]
            for i in xrange(3, MAXFD):
                try:
                    os.close(i)
                except OSError:
                    pass
            try:
                os.execvp(cmd[0], cmd)
            finally:
                os._exit(1)

    MAXFD is set as follows at the top of popen2.py:

    try:
    MAXFD = os.sysconf('SC_OPEN_MAX')
    except (AttributeError, ValueError):
    MAXFD = 256

    On AIX, SC_OPEN_MAX is 32767, whereas on Solaris, it is not defined, and we get the default value of 256. So, on AIX the python code for loop is being used to close 32763 file descriptors, but only 253 on Solaris. The slowness of this python loop is the source of the problem.

    Several solutions are possible. AIX provides a much faster way to close all file descriptors above a given one using the fcntl F_CLOSEM option. In this case, it would be: fcntl(3, F_CLOSEM, 0). Other OSes, like Solaris and BSD, have the closefrom() function instead. I think ideally, we would want to have an os.closefrom() method defined in posixmodule.c, and always available on every OS, and have popen2.py call that instead of doing the loop. The closefrom() function would be defined something like this:

    --------------------
    PyDoc_STRVAR(posix_closefrom__doc__,
    "closefrom(fd)\n\n\
    Close all file descriptors greater than or equal to fd (for low level IO).");

    static PyObject *
    posix_closefrom(PyObject *self, PyObject *args)
    {
            int fd, maxfd, res;
            if (!PyArg_ParseTuple(args, "i:closefrom", &fd))
                    return NULL;
            Py_BEGIN_ALLOW_THREADS
    
    #ifdef HAVE_CLOSEFROM
            res = closefrom(fd);
    #else
    #ifdef F_CLOSEM
            res = fcntl(3, F_CLOSEM, 0)
    #else
    
    #if defined( HAVE_SYSCONF ) && defined( _SC_OPEN_MAX )
    #  define PY_OPEN_MAX sysconf(_SC_OPEN_MAX)
    #else
    #  ifdef FOPEN_MAX
    #    define PY_OPEN_MAX FOPEN_MAX
    #  else
    #    ifdef OPEN_MAX
    #      define PY_OPEN_MAX OPEN_MAX
    #    else
    #      ifdef _NFILE
    #        define PY_OPEN_MAX _NFILE
    #      else
    #        define PY_OPEN_MAX 256
    #      endif
    #    endif
    #  endif
    #endif
            maxfd = PY_OPEN_MAX;
            while (fd < maxfd) {
              res = close(fd);
              fd++;
            }

    #endif
    #endif

            Py_END_ALLOW_THREADS
            if (res < 0)
                    return posix_error();
            Py_INCREF(Py_None);
            return Py_None;
    }

    While this is probably (close to) the ideal solution (since it would benefit all OSes by avoiding the close loop if possible or if not, moving it to C code), adding os.closefrom() probably needs to be discussed further before being accepted by the Python community.

    Instead, I will provide a simpler patch that only benefits AIX. It adds the F_CLOSEM attribute to the fcntl class in fcntlmodule.c, if defined, and modifies popen2.py to check for it and use it if possible, instead of doing the close() loop. See the attached patch (against the 2.5 source). I don't believe that any documentation has to change.

    John Allen

    @johnlallen johnlallen mannequin added stdlib Python modules in the Lib dir labels Dec 1, 2006
    @hvbargen
    Copy link
    Mannequin

    hvbargen mannequin commented Feb 19, 2007

    I have submitted a new bug and referenced this patch from there.

    I think subprocess.py should be patched in a similar way.
    But, subprocess.py uses a "but" argument in _close_fds.
    I think a correct solution should close all handles from 3 to but-1 in C using the usual "close()" API; and then close all handles from but+1 to MAX_FD by using the optimization with closefrom or fnctl.

    @schmir
    Copy link
    Mannequin

    schmir mannequin commented Feb 23, 2008

    python 2.6 has os.closerange. however it is currently not used in popen2.

    @birkenfeld
    Copy link
    Member

    Changed popen2 to use os.closerange() in r61019.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant