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:
1) We provide a new getheaderdata() function in httplib which just 
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.
2) We provide the named artgument bufsize=0 to the HTTPResponse class 
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

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?

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.

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.

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:
>
> Jeremy Hylton <jeremy@alum.mit.edu> added the comment:
>
> 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.
>
> ----------
> assignee:  -> jhylton
> nosy: +jhylton
> status: open -> pending
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4336>
> _______________________________________
> _______________________________________________
> Python-bugs-list mailing list
> Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/jeremy%40alum.mit.edu
>
>

I have addressed some issues mentioned:
1) I have retained the _send_output() method.
2) the endheaders() method now takes an optional argument, send_data 
that defaults to True.  It also returns any unsent data as a string.  
This simplifies any code wishing to send more data.
3) The old HTTP class needs no changes anymore.
4) I've fixed the test_xmlrpc.py test case to run all the tests on 
windows.  The old concern with "WSAEWOULDBLOCK" seems to be no longer 
valid.
5) concatenating the result from endheaders() with the request body is 
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.

Sorry, I meant : "you may have concerns regarding point 2) above"

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:
>
> Kristján Valur Jónsson <kristjan@ccpgames.com> added the comment:
>
> I have addressed some issues mentioned:
> 1) I have retained the _send_output() method.
> 2) the endheaders() method now takes an optional argument, send_data
> that defaults to True.  It also returns any unsent data as a string.
> This simplifies any code wishing to send more data.
> 3) The old HTTP class needs no changes anymore.
> 4) I've fixed the test_xmlrpc.py test case to run all the tests on
> windows.  The old concern with "WSAEWOULDBLOCK" seems to be no longer
> valid.
> 5) concatenating the result from endheaders() with the request body is
> 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.
>
> Added file: http://bugs.python.org/file12127/xmlrpc.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4336>
> _______________________________________
> _______________________________________________
> Python-bugs-list mailing list
> Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/jeremy%40alum.mit.edu
>
>

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.

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.

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:
>
> Kristján Valur Jónsson <kristjan@ccpgames.com> added the comment:
>
> 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.
>
> Added file: http://bugs.python.org/file12145/xmlrpc.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4336>
> _______________________________________
> _______________________________________________
> Python-bugs-list mailing list
> Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/jeremy%40alum.mit.edu
>
>

Really?
Works for me.  Here is a simple patch where we leave the read buffering.  
passes test_xmlrpclib.py and test_httplib.py

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.

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.

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.

In light of my previous patch, here is a an improvement to urllib.py and 
logging that uses endheaders(body)

Reopening this, since I am not entirely happy with the implementation 
that was checked in.  Please see my comments from Dec 5th onwards.

Checked in revision: 68458 and  
revision: 68459

note, this has been ported to Py3k in http://svn.python.org/view?
view=rev&rev=68458