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

ftplib and sendfile() #57773

Open
giampaolo opened this issue Dec 9, 2011 · 32 comments
Open

ftplib and sendfile() #57773

giampaolo opened this issue Dec 9, 2011 · 32 comments
Assignees
Labels
3.12 bugs and security fixes performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@giampaolo
Copy link
Contributor

BPO 13564
Nosy @pitrou, @giampaolo, @merwok
Dependencies
  • bpo-17552: Add a new socket.sendfile() method
  • Files
  • ftplib-sendfile.patch: initial draft
  • ftplib-sendfile2.patch: nobytes = fiilesize
  • ftplib-sendfile3.patch: use poll(); sock timeout; look for EMFILE;
  • ftplib-sendfile4.patch: do not set non-blocking; factor out support functions; improve test
  • ftplib-sendfile5.patch: uses socket.sendfile()
  • 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 = 'https://github.com/giampaolo'
    closed_at = None
    created_at = <Date 2011-12-09.06:56:06.908>
    labels = ['3.7', 'performance']
    title = 'ftplib and sendfile()'
    updated_at = <Date 2016-09-08.22:54:07.941>
    user = 'https://github.com/giampaolo'

    bugs.python.org fields:

    activity = <Date 2016-09-08.22:54:07.941>
    actor = 'christian.heimes'
    assignee = 'giampaolo.rodola'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2011-12-09.06:56:06.908>
    creator = 'giampaolo.rodola'
    dependencies = ['17552']
    files = ['23890', '29336', '29351', '29357', '35568']
    hgrepos = []
    issue_num = 13564
    keywords = ['patch']
    message_count = 32.0
    messages = ['149076', '149170', '149237', '183599', '183601', '183602', '183603', '183606', '183607', '183609', '183622', '183637', '183654', '183655', '183666', '183669', '183670', '183673', '183680', '183754', '183755', '183814', '183827', '183828', '183831', '183832', '183834', '183835', '184881', '184882', '185293', '220261']
    nosy_count = 4.0
    nosy_names = ['pitrou', 'giampaolo.rodola', 'eric.araujo', 'rosslagerwall']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue13564'
    versions = ['Python 3.6', 'Python 3.7']

    @giampaolo
    Copy link
    Contributor Author

    In attachment.
    This is actually just an excuse to store the patch somewhere and possibly collect opinions as I don't really think this should go in because:

    • it's UNIX only
    • as such, deciding whether using sendfile() should probably be done silently (no explicit argument)
    • on the other hand, I don't think it's safe to decide this automatically because:
      • the input fd should be a regular file and it's not clear how to determine this beforehand
      • in case of disconnection OSError is raised instead of socket.error (minor backward compatibility issue)

    @giampaolo giampaolo added the type-feature A feature request or enhancement label Dec 9, 2011
    @merwok
    Copy link
    Member

    merwok commented Dec 10, 2011

    deciding whether using sendfile() should probably be done silently (no explicit argument)
    As an optimization taking advantage from OS support, I think this should be automatic too. But if there are too many issues, then explicit argument sounds better.

    the input fd should be a regular file and it's not clear how to determine this beforehand
    Calling some function like os.stat that only works with real files? (not sure os.stat is the right function, just giving an idea)

    > in case of disconnection OSError is raised instead of socket.error
    PEP 3151!
    >>> socket.error, OSError, IOError
    (<class 'OSError'>, <class 'OSError'>, <class 'OSError'>)

    :)

    @pitrou
    Copy link
    Member

    pitrou commented Dec 11, 2011

    os.fstat wouldn't work since it succeeds with non-"regular" files, e.g. standard I/O:

    >>> os.fstat(0)
    posix.stat_result(st_mode=8592, st_ino=5, st_dev=11, st_nlink=1, st_uid=500, st_gid=5, st_size=0, st_atime=1323629303, st_mtime=1323629303, st_ctime=1323628616)

    I think the best solution is to call sendfile() and catch OSError, then fallback on the generic loop. However, you must also guard against fileno() failing:

    >>> io.BytesIO().fileno()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    io.UnsupportedOperation: fileno

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 6, 2013

    Here's the result of a benchmark sending a 1GB file over a Gb/s ethernet network:
    vanilla:
    real 0m9.446s
    user 0m0.493s
    sys 0m1.425s

    sendfile:
    real 0m9.143s
    user 0m0.055s
    sys 0m0.986s

    The total time doesn't vary much (the reduction above is just jitter).
    But it reduces user+sys time by almost a factor of 2.

    Note that is changed Giampaolo's patch to call sendfile on the whole file, not by block.

    @giampaolo
    Copy link
    Contributor Author

    Note that is changed Giampaolo's patch to call sendfile
    on the whole file, not by block.

    That's not compatible across POSIX platforms.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 6, 2013

    That's not compatible across POSIX platforms.

    What do you mean ?
    I juste call fstat() before calling senfile() to find out the file
    size, and pass it as count.

    @giampaolo
    Copy link
    Contributor Author

    Ah ok, I misinterpreted what you wrote then.
    Generally speaking though, you don't need to know the file size: you just decide a blocksize (>= 65536 is usually ok) and use sendfile() as you use send().

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 6, 2013

    Ah ok, I misinterpreted what you wrote then.
    Generally speaking though, you don't need to know the file size: you just decide a blocksize (>= 65536 is usually ok) and use sendfile() as you use send().

    But then you make much more syscalls than what's necessary, and your
    heuristic for the block size is never going to match the choice the
    kernel makes.

    Anyway, here are the numbers, do you think it's interesting to merge
    (I mean, you're the Python ftp expert ;-) ?

    @giampaolo
    Copy link
    Contributor Author

    Specifying a big blocksize doesn't mean the transfer will be faster.
    send/sendfile won't send more than a certain amount of bytes anyways.
    If I'm not mistaken I recall from previous benchmarks that after a certain point (131072 or something) increasing the blocksize results in equal or even worse performances.

    Another thing I don't like is that by doing so you implicitly assume that the file is "fstat-eable". I don't know if there are cases where it's not, but the less assumptions we do the better.

    Note: I'm sure that for both send() and sendfile() blocksize>=65536 is faster than blocksize=8192 (the current default) so it probably makes sense to change that (I'll file a separate issue).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 6, 2013

    Specifying a big blocksize doesn't mean the transfer will be faster.
    send/sendfile won't send more than a certain amount of bytes anyways.

    The transfer won't be faster mainly because it's really I/O bound.
    But it will use less CPU, only because you're making less syscalls.

    If I'm not mistaken I recall from previous benchmarks that after a certain point (131072 or something) increasing the blocksize results in equal or even worse performances.

    I can perfectly believe this for a send loop, maybe because you're
    exceeding the socket buffer size, or because your working set doesn't
    fit into caches anymore, etc.
    But for sendfile(), I don't see how calling it repeatedly could not be
    slower than calling it once with the overall size: that's how netperf
    and vsftpd use it, and probably others.

    Another thing I don't like is that by doing so you implicitly assume that the file is "fstat-eable". I don't know if there are cases where it's not, but the less assumptions we do the better.

    Well, the file must be mmap-able, so I doubt fstat() is the biggest concern...

    @giampaolo
    Copy link
    Contributor Author

    The transfer won't be faster mainly because it's really I/O bound.
    But it will use less CPU, only because you're making less syscalls.

    Have you actually measured this?

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 7, 2013

    > The transfer won't be faster mainly because it's really I/O bound.
    > But it will use less CPU, only because you're making less syscalls.

    Have you actually measured this?

    """
    vanilla over Gb/s:
    real 0m9.035s
    user 0m0.523s
    sys 0m1.412s

    block-sendfile over Gb/s:
    real 0m9.683s
    user 0m0.253s
    sys 0m1.212s

    full-sendfile over Gb/s:
    real 0m9.014s
    user 0m0.059s
    sys 0m1.000s
    """

    As you can see, the throughput doesn't vary (the difference in "real
    time" is just part of the variance).
    However, the CPU usage (user+sys) is less for block-sendfile than send
    loop, and less for full-sendfile than block-sendfile.

    """
    vanilla over loopback:
    real 0m3.200s
    user 0m0.541s
    sys 0m0.702s

    block-sendfile over loopback:
    real 0m2.713s
    user 0m0.248s
    sys 0m0.197s

    full-sendfile over loopback:
    real 0m1.718s
    user 0m0.055s
    sys 0m0.082s
    """

    Same thing for loopback, except that here, zero-copy makes a
    difference on the throughput because we're not I/O bound, but really
    CPU/memory bound (and here sendfile of the complete file really
    outperforms block-sendfile).

    I don't have access to a 10Gb/s network, but basic math hints that
    sendfile could make a difference on the overall throughput.

    @giampaolo
    Copy link
    Contributor Author

    It seems you're right, sorry. We need to take that into account then.

    In the meantime I rewrote the original patch and got rid of the "use_sendfile" explicit argument in order to attempt to use sendfile() by default and fall back on using send() if bytes sent were 0.

    TSL_FTP related changes are still left out for now as I'm planning a little refactoring first.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 7, 2013

    In the meantime I rewrote the original patch and got rid of the "use_sendfile" explicit argument in order to attempt to use sendfile() by default and fall back on using send() if bytes sent were 0.

    """
    # block until socket is writable
    select.select([], [sockno], [])
    """

    I don't get it, why do you use select?

    @giampaolo
    Copy link
    Contributor Author

    It's necessary because sendfile() can fail with EAGAIN.

    As for your "blocksize = filesize" argument I changed my opinion: despite being less CPU consuming we might incur into problems if that number is too big. 'count' parameter on Linux, for example, is expected to be an unsigned int.
    Other plarforms will also use different data types so we better stick with a fixed blocksize value (currently 8192).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 7, 2013

    It's necessary because sendfile() can fail with EAGAIN.

    It can fail with EAGAIN if the input FD is non-blocking, exactly like
    the current implementation which calls fp.read(). Furthermore, since
    sendfile actually supports only regular file and regular files don't
    support non-blocking I/O, it's unlikely to ever happen.

    As for your "blocksize = filesize" argument I changed my opinion: despite being less CPU consuming we might incur into problems if that number is too big. 'count' parameter on Linux, for example, is expected to be an unsigned int.

    'count' is size_t, like for mmap() and any other function accepting a
    length, so nothing wrong can happen.
    A platform which would have a sendfile prototype which doesn't support
    sending a complete file at once would be completely broken...

    @pitrou
    Copy link
    Member

    pitrou commented Mar 7, 2013

    As for your "blocksize = filesize" argument I changed my opinion:
    despite being less CPU consuming we might incur into problems if
    that number is too big. 'count' parameter on Linux, for example, is
    expected to be an unsigned int.
    Other plarforms will also use different data types so we better stick
    with a fixed blocksize value (currently 8192).

    If you really think a blocksize is necessary, you could choose a much
    larger one for sendfile() (such as 16 MB). Then the overhead of system
    calls would be much smaller.

    @giampaolo
    Copy link
    Contributor Author

    'count' is size_t, like for mmap() and any other function accepting a length, so nothing wrong can happen.

    Then why 'offset' and 'count' parameters have a different data type?

    Linux:
    sendfile(..., off_t *offset, size_t count);

    Solaris:
    sendfile(..., off_t *off, size_t len);

    HP-UX:
    sendfile(..., off_t offset, bsize_t nbytes);

    A platform which would have a sendfile prototype which doesn't support
    sending a complete file at once would be completely broken...

    You can't send a complete file at once in the first place unless it's very small.
    The usual way to send a file is chunk by chunk, so it wouldn't surprise me if sendfile() prototype does not support the use case you're describing.
    Anyway, Antoine's suggestion makes sense to me: it's probably ok to just use a big value and be done with it.
    16MB looks a little bit too much to me as the maximum amount of bytes sent per call is a lot less than 1MB, but even then it would probably be ok.

    > It's necessary because sendfile() can fail with EAGAIN.
    It can fail with EAGAIN if the input FD is non-blocking

    It will. Try it yourself.

    Furthermore, since sendfile actually supports only regular file and regular
    files don't support non-blocking I/O, it's unlikely to ever happen.

    EAGAIN is caused by the socket fd not being ready yet, not the file fd.
    Please try the patch before making such assumptions. We're going OT here.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 7, 2013

    Then why 'offset' and 'count' parameters have a different data type?

    Because offsets can be negative (e.g. for lseek), while a size can't.
    That's why 'count' is size_t, not ssize_t.

    > Furthermore, since sendfile actually supports only regular file and regular
    > files don't support non-blocking I/O, it's unlikely to ever happen.

    EAGAIN is caused by the socket fd not being ready yet, not the file fd.
    Please try the patch before making such assumptions.

    I didn't see the socket could be set to non-blocking.

    In that case, there's a problem with the patch, since select can block
    arbitrarily long because it doesn't take the socket timeout into
    account.

    Also, apparently socket.sendall() doesn't retry on EAGAIN, it doesn't
    use BEGIN_SELECT_LOOP.
    The risk of false positive (EAGAIN after select reported ready)
    shouldn't be as bad as for sendto(), since usually you'll just get a
    partial write for a stream oriented socket, but this could be bad for
    e.g. a SCTP socket (since it's message-oriented).

    We're going OT here.

    I'm leaving this topic, you can do as you like...

    @giampaolo
    Copy link
    Contributor Author

    Because offsets can be negative

    On Linux (and presumably on all POSIX platforms) passing a negative offset results in EINVAL.

    In that case, there's a problem with the patch, since select can block
    arbitrarily long because it doesn't take the socket timeout into
    account.

    Right. I will fix that.

    Also, apparently socket.sendall() doesn't retry on EAGAIN,
    it doesn't use BEGIN_SELECT_LOOP.

    socket.sendall() is not supposed to return EAGAIN in the first place. And again, I don't see how this is related with the issue at hand.

    I'm leaving this topic, you can do as you like...

    Bye.

    @giampaolo
    Copy link
    Contributor Author

    A much larger patch which should address all issues is in attachment.
    Updates:

    • use poll() instead of select() whenever possible
    • take socket timeout into account
    • take SSL/FTPS into account
    • when using select() look for EMFILE in case num fds > FD_SETSIZE
    • look for (AttributeError, io.UnsupportedOperation) when invoking file.fileno() instead of Exception, which seemed too general

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2013

    I don't understand why you must put the socket in non-blocking mode for sendfile().
    Also I think the support code (_use_send() / _use_sendfile()) should be factored out somewhere. There could even be a socket.sendfile() method with the appropriate fallbacks?

    @giampaolo
    Copy link
    Contributor Author

    I don't understand why you must put the socket in
    non-blocking mode for sendfile().

    I did that mainly because I'm using select() / poll() and it seems kind of "natural" to set the socket in non-blocking mode (proftpd does the same).
    I'm not sure whether it actually makes any difference though (on Linux it works either way, blocking or not).
    I'm removing the non-blocking mode in the new attached patch. We can take take a look at the buildbots later and see how they behave.

    Also I think the support code (_use_send() / _use_sendfile()) should be factored out somewhere.

    Agreed and turned them into methods.

    There could even be a socket.sendfile() method with the appropriate fallbacks?

    Perhaps. The only thing which is not clear is how to deal with blocking vs. non-blocking sockets.
    Also, Windows should also be covered and expose TransmitFile.
    It's probably better to discuss this elsewhere.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2013

    > I don't understand why you must put the socket in
    > non-blocking mode for sendfile().

    I did that mainly because I'm using select() / poll() and it seems
    kind of "natural" to set the socket in non-blocking mode (proftpd does
    the same).

    But why do you need to use select() / poll() ? Can't you just call
    sendfile() right from the start?

    @giampaolo
    Copy link
    Contributor Author

    Because otherwise sendfile() fails with EAGAIN many times before sending any actual data.
    select() / poll() make sure the while loop awakens only when the socket is ready to be written (as opposed to continuously catching EAGAIN and wait for sendfile() to succeed).

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2013

    Because otherwise sendfile() fails with EAGAIN many times before
    sending any actual data.

    EAGAIN on a blocking fd? Is it documented somewhere?
    The Linux man page for sendfile() says:

       EAGAIN Nonblocking I/O has been selected using O_NONBLOCK and the
    

    write would block.

    FreeBSD apparently says something similar:

     [EAGAIN]		The socket is marked for non-blocking I/O and not all
    		data was sent due to the socket buffer being filled.
    		If specified, the number of bytes successfully sent
    		will be returned in \*sbytes.
    

    @giampaolo
    Copy link
    Contributor Author

    I see. Well, what I'm experiencing right now if I remove the select() / poll() call is a sequence of EAGAIN errors alternated by successful sendfile() calls.
    Either the man page is wrong or I'm missing something.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2013

    I see. Well, what I'm experiencing right now if I remove the
    select() / poll() call is a sequence of EAGAIN errors alternated by
    successful sendfile() calls.
    Either the man page is wrong or I'm missing something.

    Weird. I guess it would be nice to dig a bit more :-)

    @giampaolo
    Copy link
    Contributor Author

    After digging a bit further it seems EAGAIN occurs in case a timeout was previously set against the socket as in ftplib.FTP(..., timeout=2) (at least on Linux, FWICT).

    As such, we can debate whether avoid using select/poll if timeout was not set.
    I'll that a look at the man pages of the other POSIX platforms and figure whether EAGAIN is interpreted as on Linux.

    Other than that, the patch is reasonably ok to me and can be committed as-is and blocksize argument tuning can be discussed in a separate ticket.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 21, 2013

    After digging a bit further it seems EAGAIN occurs in case a timeout was previously
    set against the socket as in ftplib.FTP(..., timeout=2) (at least on Linux, FWICT).

    Ah, indeed. That's because socket timeout makes the underlying fd non-blocking.
    Which means there probably should be a higher-level sendfile() facility for sockets, taking into account the socket timeout...

    @giampaolo
    Copy link
    Contributor Author

    I created bpo-17552 in order to discuss about socket.sendfile() addition.

    @giampaolo
    Copy link
    Contributor Author

    Updated patch which uses the newly added socket.sendfile() method (bpo-17552).

    @giampaolo giampaolo self-assigned this Jun 11, 2014
    @giampaolo giampaolo added performance Performance or resource usage and removed type-feature A feature request or enhancement labels Jun 11, 2014
    @tiran tiran added the 3.7 (EOL) end of life label Sep 8, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel added stdlib Python modules in the Lib dir 3.12 bugs and security fixes and removed 3.7 (EOL) end of life labels Sep 11, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.12 bugs and security fixes performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants