Issue27456
Created on 2016-07-05 15:27 by j1m, last changed 2022-04-11 14:58 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 4231 | merged | yselivanov, 2017-11-02 15:57 | |
| PR 4233 | closed | vstinner, 2017-11-02 16:09 | |
| PR 4897 | closed | yselivanov, 2017-12-16 00:44 | |
| PR 4898 | merged | yselivanov, 2017-12-16 00:46 | |
| PR 4922 | merged | yselivanov, 2017-12-19 01:32 | |
| Messages (22) | |||
|---|---|---|---|
| msg269826 - (view) | Author: Jim Fulton (j1m) * ![]() |
Date: 2016-07-05 15:27 | |
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:
https://github.com/python/asyncio/issues/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?
https://github.com/python/asyncio/issues/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.
|
|||
| msg269831 - (view) | Author: Jim Fulton (j1m) * ![]() |
Date: 2016-07-05 16:00 | |
Gaaa, forgot to set meta data. |
|||
| msg269836 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2016-07-05 16:15 | |
See also https://github.com/python/asyncio/issues/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. |
|||
| msg269837 - (view) | Author: Jim Fulton (j1m) * ![]() |
Date: 2016-07-05 16:24 | |
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. |
|||
| msg269838 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2016-07-05 16:27 | |
Fine with me -- I have no idea what this flag does (nor the desire to learn :-). |
|||
| msg269839 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2016-07-05 16:28 | |
Jim, I can make a PR, unless you want to do that. |
|||
| msg269840 - (view) | Author: Jim Fulton (j1m) * ![]() |
Date: 2016-07-05 16:30 | |
Yury, I'd be fine with you making a PR. :) Is there a similar update that can be made to uvloop? |
|||
| msg269841 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2016-07-05 16:37 | |
> 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 ;) |
|||
| msg269857 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2016-07-05 23:30 | |
PR: https://github.com/python/asyncio/pull/373 |
|||
| msg269933 - (view) | Author: Jim Fulton (j1m) * ![]() |
Date: 2016-07-07 13:14 | |
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. |
|||
| msg274251 - (view) | Author: Марк Коренберг (socketpair) * | Date: 2016-09-02 15:50 | |
vote +10 for that |
|||
| msg275907 - (view) | Author: Roundup Robot (python-dev) ![]() |
Date: 2016-09-12 01:44 | |
New changeset 3f7e4ae9eba3 by Yury Selivanov in branch '3.5': Issue #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 #27456) https://hg.python.org/cpython/rev/10384c5c18f5 |
|||
| msg275908 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2016-09-12 01:44 | |
Committed, should be in 3.6 b1. |
|||
| msg305432 - (view) | Author: Vladimir Magamedov (vmagamedov) | Date: 2017-11-02 15:48 | |
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:
- http://man7.org/linux/man-pages/man2/socket.2.html
- https://github.com/torvalds/linux/blob/v4.13/include/linux/net.h#L77
- https://github.com/freebsd/freebsd/blob/stable/11/sys/sys/socket.h#L110
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
|
|||
| msg305435 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2017-11-02 15:58 | |
> 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. |
|||
| msg305437 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2017-11-02 16:11 | |
> 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? |
|||
| msg305438 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2017-11-02 16:13 | |
_ipaddr_info() also uses "type == socket.SOCK_STREAM". Is it ok? |
|||
| msg308436 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2017-12-16 00:32 | |
New changeset e796b2fe26f220107ac50667de6cc86c82b465e3 by Yury Selivanov in branch 'master': bpo-27456: Ensure TCP_NODELAY is set on linux (#4231) https://github.com/python/cpython/commit/e796b2fe26f220107ac50667de6cc86c82b465e3 |
|||
| msg308441 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2017-12-16 02:53 | |
New changeset 572636d42566da8eb6e85d5b8164e9ed8860a255 by Yury Selivanov in branch '3.6': bpo-27456: Ensure TCP_NODELAY is set on linux (#4231) (#4898) https://github.com/python/cpython/commit/572636d42566da8eb6e85d5b8164e9ed8860a255 |
|||
| msg308627 - (view) | Author: Yury Selivanov (yselivanov) * ![]() |
Date: 2017-12-19 11:44 | |
New changeset a7bd64c0c01379e9b82e86ad41a301329a0775d9 by Yury Selivanov in branch 'master': bpo-27456: Simplify sock type checks (#4922) https://github.com/python/cpython/commit/a7bd64c0c01379e9b82e86ad41a301329a0775d9 |
|||
| msg308640 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2017-12-19 13:20 | |
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. |
|||
| msg308706 - (view) | Author: Марк Коренберг (socketpair) * | Date: 2017-12-20 07:18 | |
Note, TCP_NODELAY can not be set on UNIX streaming socket. Do you have corresponding tests for UNIX sockets ? |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:33 | admin | set | github: 71643 |
| 2017-12-20 07:18:20 | socketpair | set | messages: + msg308706 |
| 2017-12-19 13:20:05 | vstinner | set | messages: + msg308640 |
| 2017-12-19 11:44:44 | yselivanov | set | messages: + msg308627 |
| 2017-12-19 01:32:44 | yselivanov | set | pull_requests: + pull_request4817 |
| 2017-12-16 02:54:32 | yselivanov | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2017-12-16 02:53:12 | yselivanov | set | messages: + msg308441 |
| 2017-12-16 00:46:17 | yselivanov | set | pull_requests: + pull_request4792 |
| 2017-12-16 00:44:21 | yselivanov | set | pull_requests: + pull_request4791 |
| 2017-12-16 00:32:27 | yselivanov | set | messages: + msg308436 |
| 2017-11-02 16:13:23 | vstinner | set | messages: + msg305438 |
| 2017-11-02 16:11:27 | vstinner | set | messages: + msg305437 |
| 2017-11-02 16:09:36 | vstinner | set | keywords:
+ patch pull_requests: + pull_request4201 |
| 2017-11-02 15:58:37 | yselivanov | set | status: closed -> open resolution: fixed -> (no value) messages: + msg305435 stage: resolved -> patch review |
| 2017-11-02 15:57:01 | yselivanov | set | pull_requests: + pull_request4200 |
| 2017-11-02 15:48:27 | vmagamedov | set | nosy:
+ vmagamedov messages: + msg305432 |
| 2016-09-12 01:44:38 | yselivanov | set | status: open -> closed resolution: fixed messages: + msg275908 stage: resolved |
| 2016-09-12 01:44:09 | python-dev | set | nosy:
+ python-dev messages: + msg275907 |
| 2016-09-02 15:50:00 | socketpair | set | nosy:
+ socketpair messages: + msg274251 |
| 2016-09-02 11:10:06 | vstinner | set | title: TCP_NODELAY -> asyncio: set TCP_NODELAY flag by default |
| 2016-07-07 13:14:44 | j1m | set | messages: + msg269933 |
| 2016-07-05 23:30:23 | yselivanov | set | messages: + msg269857 |
| 2016-07-05 16:37:10 | yselivanov | set | messages: + msg269841 |
| 2016-07-05 16:30:43 | j1m | set | messages: + msg269840 |
| 2016-07-05 16:30:19 | gvanrossum | set | nosy:
- gvanrossum |
| 2016-07-05 16:28:13 | yselivanov | set | messages: + msg269839 |
| 2016-07-05 16:27:12 | gvanrossum | set | messages: + msg269838 |
| 2016-07-05 16:24:59 | j1m | set | messages: + msg269837 |
| 2016-07-05 16:15:00 | yselivanov | set | messages: + msg269836 |
| 2016-07-05 16:00:20 | j1m | set | versions:
+ Python 3.4, Python 3.5 nosy: + gvanrossum, vstinner, yselivanov messages: + msg269831 components:
+ asyncio |
| 2016-07-05 15:27:48 | j1m | create | |

