bpo-32221: makeipaddr: remove interface part + speedup #4724

socketpair · 2017-12-05T05:32:17Z

https://bugs.python.org/issue32221

recvfrom from multicast socket is painfull slow. In fact, it returns sender address in form:

('fe80::941f:f6ff:fe04:c560%qwe', 42133, 0, 78)
which is superfluous, since interface-name part (%qwe) is not actually used. Actually, scopeid (78) signify interface/scope/zone_id. This tuple can be used for .sendto() either with this interface-name-part or without.

The problem is in the performance. For each socket.recvrfom(), libc.getnameinfo() internally converts interface index to interface name using three syscalls, i.e. socket(), getsockopt()?, close() , which slows down receiving (I have not measured result, but see additional syscalls in strace).

In order to convert from tuple to string-based full address one may use socket.getnameinfo():
As you can see, initial interface is ignored (but without my patch it is also validated uselessly):

In[1]: socket.getnameinfo(('fe80::941f:f6ff:fe04:c560%qwe', 42133, 0, 78), socket.NI_NUMERICHOST)
Out[1]: ('fe80::941f:f6ff:fe04:c560%qwe', '42133')
In[2]: socket.getnameinfo(('fe80::941f:f6ff:fe04:c560', 42133, 0, 78), socket.NI_NUMERICHOST)
Out[2]: ('fe80::941f:f6ff:fe04:c560%qwe', '42133')

asvetlov · 2017-12-05T09:08:21Z

Would you add a test for new functionality?

socketpair · 2017-12-05T11:02:30Z

@asvetlov done

socketpair · 2017-12-05T11:03:00Z

Doc/library/socket.rst

@@ -77,6 +77,11 @@ created.  Socket addresses are represented as follows:
  backward compatibility.  Note, however, omission of *scopeid* can cause problems
  in manipulating scoped IPv6 addresses.

+  .. versionchanged:: 3.6


I don't know which version to write here.

3.6 is in bugfix mode.
The PR should land into 3.7 only IMHO

This is 3.7 only.

asvetlov · 2017-12-06T08:37:35Z

Please add NEWS.d entry

socketpair · 2017-12-06T11:32:41Z

Is there anybody who can make a review ?

socketpair · 2017-12-14T14:54:35Z

@asvetlov could you please speedup review of this PR somehow ?

socketpair · 2017-12-20T11:33:41Z

Original (not patched) python:

In [1]s = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
In [2]: s.bind(('ff02::1de:c0db', 1234, 0, 2))
In [3]: timeit s.getsockname()
The slowest run took 12.06 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 3.8 µs per loop

In [5]: d = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
In [6]: d.bind(('::', 1235, 0, 0))
In [7]: timeit d.getsockname()
The slowest run took 23.18 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 532 ns per loop

On patched version, times for both cases should be the same exactly (i.e. 532 ns)

1st1 · 2017-12-20T16:19:17Z

Modules/socketmodule.c

 {
-    char buf[NI_MAXHOST];
-    int error;
+    char buf[(INET_ADDRSTRLEN > INET6_ADDRSTRLEN ? INET_ADDRSTRLEN : INET6_ADDRSTRLEN) + 1];


Use Py_MAX

1st1 · 2017-12-20T16:20:33Z

Modules/socketmodule.c

+            ptr = &((const struct sockaddr_in6*) addr)->sin6_addr;
+            break;
+        default:
+            abort();


I actually want an error set and NULL returned here.

this is impossible to trigger. It's part of C logic, so no exception can be imagined here. It should be like assert(0). Or, even, I can remove default: branch.

This function is called (and should be) only from AF_INET and AF_INET6 branches from upper code.

Then rewrite this to:

if (family == AF_INET) { ... } else { assert(family == AF_INET6); ... }

1st1 · 2017-12-20T16:23:09Z

Overall looks fine, please fix the couple of things I left my comments on.

socketpair · 2017-12-20T20:37:54Z

I have found two bugs in my patch:

It did not check #ifdef ENABLE_IPV6
makeipaddr had unused argument addrlen.

Since it hard to code Py_MAX + ifdef and also difficulties with assert() I decide to split this function to two. Actually, all places who call it know address family, so automatic logic is not needed.

Please have a look.

@1st1

1st1 · 2017-12-20T20:44:46Z

Modules/socketmodule.c

@@ -1097,23 +1097,35 @@ setipaddr(const char *name, struct sockaddr *addr_ret, size_t addr_ret_size, int


 /* Create a string object representing an IP address.


Remove this comment, it's outdated.

Add a comment to make_ipv4_addr "Convert IPv4 sockaddr to a Python str." Same for make_ipv6_addr.

1st1 · 2017-12-20T20:45:41Z

I decide to split this function to two.

I like it.

socketpair · 2017-12-20T21:03:33Z

@1st1 done

socketpair · 2017-12-20T21:23:32Z

After my patch:

$ python3.7 -m timeit --setup 'import socket; s = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM); s.bind(("ff02::1de:c0db", 1234, 0, 2))' 's.getsockname()'
500000 loops, best of 5: 613 nsec per loop

$ python3.7 -m timeit --setup 'import socket; s = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM); s.bind(("::", 1235, 0, 0))' 's.getsockname()'
500000 loops, best of 5: 420 nsec per loop

asvetlov

Second iteration looks much better!

asvetlov · 2017-12-22T09:40:25Z

@1st1 please review again

1st1 · 2017-12-22T12:13:36Z

@asvetlov the pr looks fine, but please don't merge it, I'm still not sure if it's backwards compatible "enough", and I don't have time for research right now.

asvetlov · 2017-12-22T12:29:09Z

Ok, let's wait for a while

socketpair · 2017-12-28T13:36:46Z

@1st1 ping

socketpair · 2018-01-02T11:08:57Z

How can I speed up merging ?

socketpair · 2018-01-02T11:14:08Z

Modules/socketmodule.c

-        set_gaierror(error);
+    char buf[INET_ADDRSTRLEN + 1];
+    if (inet_ntop(AF_INET, &addr->sin_addr, buf, sizeof(buf)) == NULL) {
+        PyErr_SetFromErrno(PyExc_OSError);
        return NULL;
    }
    return PyUnicode_FromString(buf);


Will PyUnicode_DecodeASCII be faster ? Is it better to use it here ?

I think yes.

socketpair · 2018-01-05T18:01:53Z

Modules/socketmodule.c

@@ -5911,14 +5916,11 @@ socket_inet_ntop(PyObject *self, PyObject *args)
    Py_buffer packed_ip;
    const char* retval;
 #ifdef ENABLE_IPV6
-    char ip[Py_MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) + 1];


It is not related to my issue, but this is a cosmetic fix.

socketpair · 2018-01-07T14:56:31Z

Regarding meltdown vulnerability: this patch will speed up even more!

socketpair · 2018-01-07T14:58:31Z

Because 3 syscalls per addr serialisation eliminated.

socketpair · 2018-01-15T12:59:02Z

@1st1 @asvetlov maybe someone else can merge this ?

asvetlov · 2018-01-15T14:28:57Z

The pr is good for me but @1st1 explicitly asked for additional time.

1st1 · 2018-01-15T14:54:59Z

I'll take a look this week.

socketpair · 2018-01-22T09:10:43Z

@1st1 ping

socketpair · 2018-01-26T16:37:03Z

@1st1
@asvetlov

1st1 · 2018-01-26T16:47:31Z

i'll take a look today/tomorrow.

1st1 · 2018-01-26T16:48:42Z

In the meanwhile, can you explain why this patch is important? and what is the speedup?

socketpair · 2018-01-26T17:45:49Z

@1st1
#4724 (comment)
#4724 (comment)

This patch speeds up all functions that return 4-item tuple with address. Old code uses wrong function to convert binary representation of IPv6 address to textual one.

For non-scoped addresses result is exactly the same. But for scoped addresses this wrong function does useless interface index to name resolution. This slows down for example recvfrom(). This slowdown is significant for multicast UDP applications (like my own :) )

…honGH-4724)" This reverts commit 47c0b1f.

…4724)" (#5394) This reverts commit 47c0b1f.

bedevere-bot added the awaiting review label Dec 5, 2017

the-knights-who-say-ni added the CLA signed label Dec 5, 2017

socketpair changed the title ~~makeipaddr: remove interface part + speedup~~ bpo-32221: makeipaddr: remove interface part + speedup Dec 5, 2017

socketpair commented Dec 5, 2017

View reviewed changes

1st1 reviewed Dec 20, 2017

View reviewed changes

1st1 self-assigned this Dec 20, 2017

1st1 reviewed Dec 20, 2017

View reviewed changes

asvetlov approved these changes Dec 20, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 20, 2017

socketpair commented Jan 2, 2018

View reviewed changes

bpo-32221: makeipaddr(): remove interface part + speedup

2f725fb

socketpair commented Jan 5, 2018

View reviewed changes

1st1 merged commit 47c0b1f into python:master Jan 27, 2018

bedevere-bot removed the awaiting merge label Jan 27, 2018

1st1 added a commit to 1st1/cpython that referenced this pull request Jan 28, 2018

Revert "bpo-32221: makeipaddr(): remove interface part + speedup (pyt…

2498ab4

…honGH-4724)" This reverts commit 47c0b1f.

1st1 added a commit that referenced this pull request Jan 28, 2018

Revert "bpo-32221: makeipaddr(): remove interface part + speedup (GH-…

0ceb717

…4724)" (#5394) This reverts commit 47c0b1f.

		@@ -1097,23 +1097,35 @@ setipaddr(const char name, struct sockaddr addr_ret, size_t addr_ret_size, int


		/* Create a string object representing an IP address.

bpo-32221: makeipaddr: remove interface part + speedup #4724

bpo-32221: makeipaddr: remove interface part + speedup #4724

Conversation

socketpair commented Dec 5, 2017 • edited Loading

asvetlov commented Dec 5, 2017

socketpair commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Dec 6, 2017

socketpair commented Dec 6, 2017

socketpair commented Dec 14, 2017 • edited Loading

socketpair commented Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Dec 20, 2017

socketpair commented Dec 20, 2017

Choose a reason for hiding this comment

1st1 commented Dec 20, 2017

socketpair commented Dec 20, 2017

socketpair commented Dec 20, 2017

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov commented Dec 22, 2017

1st1 commented Dec 22, 2017

asvetlov commented Dec 22, 2017

socketpair commented Dec 28, 2017

socketpair commented Jan 2, 2018

socketpair Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair commented Jan 7, 2018

socketpair commented Jan 7, 2018 • edited Loading

socketpair commented Jan 15, 2018

asvetlov commented Jan 15, 2018

1st1 commented Jan 15, 2018

socketpair commented Jan 22, 2018

socketpair commented Jan 26, 2018

1st1 commented Jan 26, 2018

1st1 commented Jan 26, 2018

socketpair commented Jan 26, 2018 • edited Loading

socketpair commented Dec 5, 2017 •

edited

Loading

socketpair commented Dec 14, 2017 •

edited

Loading

socketpair commented Dec 20, 2017 •

edited

Loading

socketpair Dec 20, 2017 •

edited

Loading

socketpair Jan 2, 2018 •

edited

Loading

socketpair commented Jan 7, 2018 •

edited

Loading

socketpair commented Jan 26, 2018 •

edited

Loading