| msg178212 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2012-12-26 13:51 |
| As noted in issue #12498, asyncore.dispatcher_with_send only sends 512 bytes at a time: this is 1/3 of the Ethernet MTU, which reduces greatly the attainable bandwidth and increases the CPU usage. Here's a patch bumping it to 8192 (and making it a class member so that derived classes can customize it if needed, although not documented). Here's the result of a simplistic benchmark using asyncore.dispatcher_with_send to send data to a server: Without patch: """ $ time ./python ~/test_asyncore.py localhost 4242 real 0m6.098s user 0m4.472s sys 0m1.436s """ With patch: """ $ time ./python ~/test_asyncore.py localhost 4242 real 0m0.937s user 0m0.796s sys 0m0.112s """ Of course, this is using the loopback interface, but it shows that the performance gain can non negligible. The test script is attached (use "netcat -l -p 4242" as server). |
|
|
| msg178214 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-26 14:00 |
| A couple of years ago I conducted similar tests and it turned out that 64k was the best compromise: http://code.google.com/p/pyftpdlib/issues/detail?id=94 Twisted uses 128k. I'd be for using 64k and also change asynchat.async_chat.ac_*_buffer_size in accordance. |
|
|
| msg178389 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-12-28 16:35 |
| > A couple of years ago I conducted similar tests and it turned out that 64k was the best compromise: > http://code.google.com/p/pyftpdlib/issues/detail?id=94 > Twisted uses 128k. > I'd be for using 64k and also change asynchat.async_chat.ac_*_buffer_size in accordance. That sounds reasonable. However, the mere fact that such constants are duplicated tends to make me think that async_chat should actually be a subclass of asyncore.asyncore_with_send (or move c_*_buffer_size to asyncore.asyncore). Sane default buffering constants belong more to asyncore than asynchat, no? |
|
|
| msg178396 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-28 17:05 |
| IMO no. asyncore.dispatcher_with_send should not exist in the first place as it basically is a castrated version of asynchat.async_chat with less capabilities. I'd say it's there only for an historical reason. Moving ac_*_buffer_size to asyncore.dispatcher makes no sense as it offers no buffer capabilities whatsoever in the first place. |
|
|
| msg178400 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-12-28 17:28 |
| I agree that 64K seems better here (on Linux). There is another problem with dispatcher_with_send: the buffering algorithm (both when appending and popping) is quadratic. You can easily observe it with your test script, when growing the DATA. async_chat looks much saner in that respect, I wonder why the same algorithm couldn't it be re-used. (regardless, reading the asyncore code really hurts the eyes :-/) |
|
|
| msg178403 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-12-28 17:49 |
| Alright, here's a simple patch bumping the buffers to 64K for asyncore and asynchat. > There is another problem with dispatcher_with_send: the buffering algorithm (both when appending and popping) is quadratic. You can easily observe it with your test script, when growing the DATA. async_chat looks much saner in that respect, I wonder why the same algorithm couldn't it be re-used. Yeah, I noticed that. But even in asynchat, there's a lot of copying going on, length computations performed twice in a row, etc. > (regardless, reading the asyncore code really hurts the eyes :-/) Indeed. |
|
|
| msg178407 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-28 18:05 |
| > But even in asynchat, there's a lot of copying going on, length > computations performed twice in a row, etc. What/where do you mean exactly? I see little value in focusing efforts towards things such as initiate_with_send which are not supposed to be used but asynchat.async_chat is different. |
|
|
| msg178413 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-12-28 18:45 |
| > What/where do you mean exactly? """ 187 def push (self, data): 188 sabs = self.ac_out_buffer_size 189 if len(data) > sabs: 190 for i in range(0, len(data), sabs): 191 self.producer_fifo.append(data[i:i+sabs]) """ len(data) is called twice """ 228 # handle classic producer behavior 229 obs = self.ac_out_buffer_size 230 try: 231 data = first[:obs] 232 except TypeError: 233 data = first.more() """ Here, I think that len(first) <= self.ac_out_buffer_size, by definition. So the slicing is actually just a copy (I realize that it has the side effect of checking whether it's a buffer or a producer). memoryview is also great to avoid copies when sending/receiving to a socket. |
|
|
| msg178415 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-28 19:27 |
| > memoryview is also great to avoid copies when sending/receiving to a socket. That's interesting. How exactly? Would producer_fifo have to change from a deque() to a memoryview() object? In that case we might have to take backward compatibility into account (producer_fifo already changed in 2.6 and if I'm not mistaken that might have caused some problems). Maybe it makes sense to file a separate issue to address asynchat enhancements. |
|
|
| msg178434 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-12-28 21:45 |
| >> memoryview is also great to avoid copies when sending/receiving to a socket. > > That's interesting. How exactly? Would producer_fifo have to change from a deque() to a memoryview() object? In that case we might have to take backward compatibility into account (producer_fifo already changed in 2.6 and if I'm not mistaken that might have caused some problems). > Maybe it makes sense to file a separate issue to address asynchat enhancements. Probably. For an example of how it might be used, you can have a look here: http://stackoverflow.com/questions/6736771/buffers-and-memoryview-objects-explained-for-the-non-c-programmer and here: http://eli.thegreenplace.net/2011/11/28/less-copies-in-python-with-the-buffer-protocol-and-memoryviews/ Also, IIRC, Antoine used memoryviews when rewriting parts of multiprocessing in pure Python. |
|
|
| msg178436 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-12-28 21:49 |
| > Would producer_fifo have to change from a deque() to a memoryview() > object? A memoryview is not a container, it's a view over an existing container. > In that case we might have to take backward compatibility into account > (producer_fifo already changed in 2.6 and if I'm not mistaken that > might have caused some problems). Does asyncore expose its implementation details? |
|
|
| msg178693 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-31 15:37 |
| > Does asyncore expose its implementation details? I was talking about asynchat. What is supposed to change is asynchat.async_chat.producer_fifo attribute which is currently a collections.deque object. |
|
|
| msg178694 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2012-12-31 15:37 |
| BTW, the patch looks ok to me. |
|
|
| msg178734 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-01-01 15:33 |
| New changeset 2eddf7c2efe6 by Charles-François Natali in branch 'default': Issue #16787: Increase asyncore and asynchat default output buffers size, to http://hg.python.org/cpython/rev/2eddf7c2efe6 |
|
|
| msg178735 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-01-01 15:45 |
| Closing! |
|
|