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

asyncio: set TCP_NODELAY flag by default #71643

Closed
jimfulton mannequin opened this issue Jul 5, 2016 · 22 comments
Closed

asyncio: set TCP_NODELAY flag by default #71643

jimfulton mannequin opened this issue Jul 5, 2016 · 22 comments
Labels
expert-asyncio performance

Comments

@jimfulton
Copy link
Mannequin

@jimfulton jimfulton mannequin commented Jul 5, 2016

BPO 27456
Nosy @vstinner, @jimfulton, @vmagamedov, @socketpair, @1st1
PRs
  • #4231
  • #4233
  • #4897
  • #4898
  • #4922
  • 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 2017-12-16.02:54:32.507>
    created_at = <Date 2016-07-05.15:27:48.626>
    labels = ['expert-asyncio', 'performance']
    title = 'asyncio: set TCP_NODELAY flag by default'
    updated_at = <Date 2017-12-20.07:18:20.545>
    user = 'https://github.com/jimfulton'

    bugs.python.org fields:

    activity = <Date 2017-12-20.07:18:20.545>
    actor = 'socketpair'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-12-16.02:54:32.507>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2016-07-05.15:27:48.626>
    creator = 'j1m'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 27456
    keywords = ['patch']
    message_count = 22.0
    messages = ['269826', '269831', '269836', '269837', '269838', '269839', '269840', '269841', '269857', '269933', '274251', '275907', '275908', '305432', '305435', '305437', '305438', '308436', '308441', '308627', '308640', '308706']
    nosy_count = 6.0
    nosy_names = ['vstinner', 'j1m', 'vmagamedov', 'socketpair', 'python-dev', 'yselivanov']
    pr_nums = ['4231', '4233', '4897', '4898', '4922']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue27456'
    versions = ['Python 3.4', 'Python 3.5']

    @jimfulton
    Copy link
    Mannequin Author

    @jimfulton jimfulton mannequin commented Jul 5, 2016

    tl;dr TCP_NODELAY should be set by default and/or there should be a
    proper way to set it.

    I've ported ZEO, ZODB's client-server networking layer to asyncio.
    Things were going pretty well. I've been developing on a
    Mac. Yesterday, I ran some performance measurements on Linux and found
    some tests ran 30x slower.

    After some digging, I came across this:

    python/asyncio#311

    Then led me to try setting TCP_NODELAY, which provided Linux
    performance comparable to Mac OS performance.

    Issue 311 suggested that this was a kernal-version issue. I think
    this is a failure to set, or at least provide a way to set a "don't be
    stupid" option.

    I originally tried this on Ubuntu 14.04, with kernal
    3.13.0-74-generic. I then tried Ubuntu 16.04, in a docker image, with
    kernal 4.4.12-boot2docker. For both of these, performance for the
    write tests were 30x slower unless I set TCP_NODELAY.

    Finally, I tried an Amazon standard AMI, which runs some version of
    RedHat, with kernal 4.4.11-23.53.amzn1.x86_64. On that system,
    performance of the tests was similar to Mac OS X without setting
    TCP_NODELAY.

    Note that the non-slow kernal version was lower than the slow (Ubuntu)
    one. I don't think this is mearly a kernal version issue, nor do I
    think this should have been dismissed as one.

    I couldn't find a way to set TCP_NODELAY cleanly. Did I miss something?
    python/asyncio#286 suggests there isn't one.

    I ended up having to set the option on _sock transport attributes,
    which is dirty and, I assume, won't work with uvloop. (BTW, uvloop was
    only ~15x slower on linux systems with this problem.)

    I think TCP_NODELAY should be set by default. Perhaps it shouldn't be
    set on mobile, by everywhere else, I think it's a "don't be stupid"
    option.

    I also think there should be a way to set it cleanly.

    IMO, this is extremely important. Linux is a wildly important platform
    for networking applications and Python, and, for better or worse,
    Ubuntu is a very commonly used distribution. Having asyncio, perform
    so poorly on these platforms is a big deal.

    @jimfulton
    Copy link
    Mannequin Author

    @jimfulton jimfulton mannequin commented Jul 5, 2016

    Gaaa, forgot to set meta data.

    @jimfulton jimfulton mannequin added expert-asyncio performance labels Jul 5, 2016
    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jul 5, 2016

    See also python/asyncio#286

    I also wanted to raise this question. I think transports should set NODELAY by default (as Golang, for instance). I've been hit by this thing a few times already -- performance of protocols over TCP is terrible because somebody forgot to set this flag.

    @jimfulton
    Copy link
    Mannequin Author

    @jimfulton jimfulton mannequin commented Jul 5, 2016

    I forgot to switch to the asyncio branch of ZEO when testing on the Amazon/RedHat linux. When I do, the tests run slow there too. Asyncio is dog slow on every linux I've tried.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jul 5, 2016

    Fine with me -- I have no idea what this flag does (nor the desire to learn :-).

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jul 5, 2016

    Jim, I can make a PR, unless you want to do that.

    @jimfulton
    Copy link
    Mannequin Author

    @jimfulton jimfulton mannequin commented Jul 5, 2016

    Yury, I'd be fine with you making a PR. :)

    Is there a similar update that can be made to uvloop?

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jul 5, 2016

    Is there a similar update that can be made to uvloop?

    Yes. Once it's committed to asyncio, I'll add it to uvloop immediately. That's one benefit of using uvloop -- it gets updates faster ;)

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jul 5, 2016

    @jimfulton
    Copy link
    Mannequin Author

    @jimfulton jimfulton mannequin commented Jul 7, 2016

    I missed the point that you can get a transport's socket using get_extra_info. IMO, this provides an adequate escape from the default and qualifies as a "proper way to set it".

    I still think the default should be to enable TCP_NODELAY.

    @vstinner vstinner changed the title TCP_NODELAY asyncio: set TCP_NODELAY flag by default Sep 2, 2016
    @socketpair
    Copy link
    Mannequin

    @socketpair socketpair mannequin commented Sep 2, 2016

    vote +10 for that

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Sep 12, 2016

    New changeset 3f7e4ae9eba3 by Yury Selivanov in branch '3.5':
    Issue bpo-27456: asyncio: Set TCP_NODELAY by default.
    https://hg.python.org/cpython/rev/3f7e4ae9eba3

    New changeset 10384c5c18f5 by Yury Selivanov in branch 'default':
    Merge 3.5 (issue bpo-27456)
    https://hg.python.org/cpython/rev/10384c5c18f5

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Sep 12, 2016

    Committed, should be in 3.6 b1.

    @1st1 1st1 closed this as completed Sep 12, 2016
    @vmagamedov
    Copy link
    Mannequin

    @vmagamedov vmagamedov mannequin commented Nov 2, 2017

    Seems like this fix is incomplete. It contains this check:

    sock.type == socket.SOCK_STREAM
    

    But sock.type is not only a type (at least in Linux and FreeBSD), it also may contain SOCK_NONBLOCK and SOCK_CLOEXEC flags. So I'm hitting the same problem: on the Linux in asyncio I have:

    > sock.type == socket.SOCK_STREAM | socket.SOCK_NONBLOCK == 2049
    True
    

    So this check isn't working and TCP_NODELAY still disabled by default.

    Links:

    Linux has SOCK_TYPE_MASK definition equal to 0xf, but I can't find such definition in the FreeBSD sources. And I don't know how to reliably and with forward compatibility check sock.type without calling getsockopt() syscall.

    Currently I have a fix in my project, where:

        _sock_type_mask = 0xf if hasattr(socket, 'SOCK_NONBLOCK') else 0xffffffff

    And then in my own _set_nodelay(sock) function:

    sock.type & _sock_type_mask == socket.SOCK_STREAM
    

    Should I make a pull request or someone knows more reliable check? Or it is ok to add one more syscall?

    sock.getsockopt(socket.SOL_SOCKET, socket.SO_TYPE) == socket.SOCK_STREAM
    

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Nov 2, 2017

    Seems like this fix is incomplete. It contains this check:

    sock.type == socket.SOCK_STREAM

    But sock.type is not only a type

    Good catch. I made a PR to fix this.

    @1st1 1st1 reopened this Nov 2, 2017
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 2, 2017

    sock.type == socket.SOCK_STREAM

    Oh, right. I wrote PR 4233 to fix the code.

    Yury: "Good catch. I made a PR to fix this."

    Oops. If you wrote a PR, would it mind to compare it with mine, please?

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 2, 2017

    _ipaddr_info() also uses "type == socket.SOCK_STREAM". Is it ok?

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Dec 16, 2017

    New changeset e796b2f by Yury Selivanov in branch 'master':
    bpo-27456: Ensure TCP_NODELAY is set on linux (bpo-4231)
    e796b2f

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Dec 16, 2017

    New changeset 572636d by Yury Selivanov in branch '3.6':
    bpo-27456: Ensure TCP_NODELAY is set on linux (bpo-4231) (bpo-4898)
    572636d

    @1st1 1st1 closed this as completed Dec 16, 2017
    @1st1
    Copy link
    Member

    @1st1 1st1 commented Dec 19, 2017

    New changeset a7bd64c by Yury Selivanov in branch 'master':
    bpo-27456: Simplify sock type checks (bpo-4922)
    a7bd64c

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 19, 2017

    Yury: thanks for fixing socket.socket.type. asyncio code now looks simpler and easier to follow without the helper functions just to test the socket type for equality.

    @socketpair
    Copy link
    Mannequin

    @socketpair socketpair mannequin commented Dec 20, 2017

    Note, TCP_NODELAY can not be set on UNIX streaming socket. Do you have corresponding tests for UNIX sockets ?

    @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
    expert-asyncio performance
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants