Repository navigation
Fix performance issues in xmlrpclib #48586
Description
| BPO | 4336 |
|---|---|
| Nosy | @kristjanvalur, @vstinner |
| Files |
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 2009-01-09.20:27:40.676>
created_at = <Date 2008-11-17.14:33:40.158>
labels = ['extension-modules', 'easy', 'performance']
title = 'Fix performance issues in xmlrpclib'
updated_at = <Date 2009-01-19.10:29:43.373>
user = 'https://github.com/kristjanvalur'bugs.python.org fields:
activity = <Date 2009-01-19.10:29:43.373>
actor = 'kristjan.jonsson'
assignee = 'jhylton'
closed = True
closed_date = <Date 2009-01-09.20:27:40.676>
closer = 'kristjan.jonsson'
components = ['Extension Modules']
creation = <Date 2008-11-17.14:33:40.158>
creator = 'kristjan.jonsson'
dependencies = []
files = ['12030', '12127', '12144', '12145', '12178', '12282', '12283']
hgrepos = []
issue_num = 4336
keywords = ['patch', 'easy']
message_count = 19.0
messages = ['75962', '76174', '76343', '76346', '76349', '76406', '76407', '76489', '76502', '76523', '76578', '76682', '76697', '77024', '77309', '77310', '79407', '79502', '80153']
nosy_count = 4.0
nosy_names = ['jhylton', 'kristjan.jonsson', 'vstinner', 'hodgestar']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue4336'
versions = ['Python 2.7']
Activity
kristjanvalur commentedon Nov 17, 2008
There are two performance problems in xmlrpclib.py:
POST requests use two send() calls, one to send the headers and one to
send the data. This can invoke the Nagle/Delayed ACK performance
problem. On many socket implementations (including some windows
platforms) it can introduce delays of up to 200ms when talking over the
wire (as opposed to localhost)
The second is the use of no buffering when reading the xmlrpc
response. It is done using the readline() call on a file-like object
constructed from the socket. When no buffering is in effect, this
causes a separate recv() call for each character.
This patch fixes these issues separately:
returns the data for the headers instead of sending it. This can be
used instead of endheaders() if the intention is to subsequently send
more data. xmlrpclib then uses this method of framing its POST
messages.
in httplib, and also so on for the HTTPConnection.getresponse() and
HTTP.getreply(). xmlrpclib then passes bufsize=-1 to HTTP.getreply() to
get default buffering for the response fileobject.
This patch focuses on fixing the problems with xmlrpclib. but issue 1)
above also has a number of other manifestations in the lib, there are
other places where we can use getheaderdata() and send() instead of
endheaders() to avoid possible Nagle/Ack problems, e.g. in urllib.py,
distutils.py and loggin/handlers.py
kristjanvalur commentedon Nov 21, 2008
Just a thought here:
Maybe it would be better just to change socket._fileobject to always
use a minimum of 8k readbuffer for readline() just as read() already
does. The read() documentation states that recv(1) is very
inefficient, and so it is.
The intent, when calling makefile(bufsize=0) is probably to make sure
that buffering for write() is disabled.
Any thoughts?
jhylton commentedon Nov 24, 2008
This patch makes sense in principle, but some of the details need to
change. The _send_output() method is used by some clients, merely
because it can be used :-(. It's fairly easy to preserve this API for
backwards compatibility.
I am also worried about this new api call getheaderdata(). It
complicates the API. Even if it were necessary, it needs a better name,
because getheaderdata() doesn't sound like a method that changes the
connection state or consumes buffered header data.
I think it would be better to have the new behavior exposed only through
HTTPConnection and not HTTP, since that's a Python 1.5.2 compatibility
API(!). We can make some small changes to xmlrpclib to use the newer
API. It would probably be a good change merely because py3k now uses
the newer API.
I've got a working local change, but it's still a little ugly.
jhylton commentedon Nov 24, 2008
Just wanted to mention that the best solution is to update as much code
as possible to use HTTPConnection instead of HTTP. I'm not sure how
easy it is to do for xmlrpclib, since it exposes methods like
send_content(). I guess we can preserve those APIs somehow, but it
won't be pretty.
jhylton commentedon Nov 24, 2008
Actually, the patch doesn't work :-). The httplib API allows you to
pass a file or a string for the body of the request. The
getheaderdata() + body case only works for the string. Easy to fix,
but I worry that he still run into Nagle problems with the file case,
which was presumably added for efficiency.
Jeremy
On Mon, Nov 24, 2008 at 12:32 PM, Jeremy Hylton <report@bugs.python.org> wrote:
kristjanvalur commentedon Nov 25, 2008
I have addressed some issues mentioned:
that defaults to True. It also returns any unsent data as a string.
This simplifies any code wishing to send more data.
windows. The old concern with "WSAEWOULDBLOCK" seems to be no longer
valid.
valid, in xmlrpclib, because the assumption has already been made in
send_content() that request_body is a string (str(len(request_body)).
However, in httplib's request() method, we now special case this. I
don't want to complicate the code for what appears to be a rare case.
I chose not to mess with xmlrpclib and make it continue to use the old
HTTP class in order to minimise the extent of this patch. A simple and
clear performance patch has in my experience a much greater chance of
finding its way into the codebase than a more extensive rewrite :)
You may have concerns regarding point 3 above, and I am open to
suggestions.
Now, what remains is the question of the read buffering for the socket
fileobject. Do you think that it is feasible to simply change the
default read buffering for fileobjects to use buffering for readline()
same as they do for read()? Then we wouldn't need the second half of
this patch and we could make a separate "socket" performance patch.
kristjanvalur commentedon Nov 25, 2008
Sorry, I meant : "you may have concerns regarding point 2) above"
jhylton commentedon Nov 26, 2008
I think we're making progress, but I'm still not sure about the new
httplib api. My current worry is that endheaders() behaves very
differently when send_data is false. My chief concern is that the
__state variable is going to indicate that the request has been sent
when we're really depending on the client to send the header. In
general, I don't like the public send() method since it's exposing a
raw socket that could be called at any time regardless of the _state
of the object.
What if endheaders() takes body as an optional argument and sends it
along with the headers? This accomplishes the same basic goal as
returning the header from endheaders(), but keeps the actual sending
of data encapsulated within the http connection.
Jeremy
On Tue, Nov 25, 2008 at 6:13 AM, Kristján Valur Jónsson
<report@bugs.python.org> wrote:
kristjanvalur commentedon Nov 27, 2008
I like the suggestion of having endheaders() accept any extra data.
I have uploaded a new patch which implements this idea. It simplifies
things a lot.
The issue with the read buffer size is still open. I have sent an
email to python-dev requesting comments.
kristjanvalur commentedon Nov 28, 2008
Guido pointed out the problem with _fileobject.readline() being
followed by socket.recv() if readline uses read buffering.
xmlrpclib.py was attempting to directly use the underlying socket,
although in actual fact it never did, (since HTTPConnectio had closed
it when it returned the response from getresponse()) Never the less, it
is prudent to make sure that we don't attempt this.
There really should be no need to use the socket directly, a buffered
read() call is just as efficient.
jhylton commentedon Nov 29, 2008
I did the simple part of the patch, where the request and headers are
sent at the same time. The applied patch didn't pass the test suite,
and I want to think about the buffering change a bit more. It's
definitely tricky.
Jeremy
On Fri, Nov 28, 2008 at 5:53 AM, Kristján Valur Jónsson
<report@bugs.python.org> wrote:
kristjanvalur commentedon Dec 1, 2008
Really?
Works for me. Here is a simple patch where we leave the read buffering.
passes test_xmlrpclib.py and test_httplib.py
jhylton commentedon Dec 1, 2008
I submitted r67442, which combines the headers and body in a single
send() call. We should look at the buffering issue now, although I
probably won't have any time to check on it until Friday.
kristjanvalur commentedon Dec 5, 2008
I think it would have been better to have endheaders() (and then perhaps
_send_output()) deal with the non-string (i.e. filebuffer) case, so
that endheaders(body) is semantically equivalent to endheaders();
send(body). The version you checked in makes it necessary that the
user of HTTPConnection.endheaders() is aware of the distinction.
kristjanvalur commentedon Dec 8, 2008
Please consider applying this patch. It moves the logic handling of
non-string message_body inside of endheaders(), allowind clients of
HTTPConnection to substitute endheader();send(data) with
endheaders(data) without fear.
kristjanvalur commentedon Dec 8, 2008
In light of my previous patch, here is a an improvement to urllib.py and
logging that uses endheaders(body)
kristjanvalur commentedon Jan 8, 2009
Reopening this, since I am not entirely happy with the implementation
that was checked in. Please see my comments from Dec 5th onwards.
kristjanvalur commentedon Jan 9, 2009
Checked in revision: 68458 and
revision: 68459
kristjanvalur commentedon Jan 19, 2009
note, this has been ported to Py3k in http://svn.python.org/view?
view=rev&rev=68458