| msg65121 - (view) |
Author: Aren Olson (reacocard) |
Date: 2008-04-07 20:55 |
| This is a reposting of issue 508157, as requested by gvanrossum. The socket file object in httplib is opened without any buffering resulting in very slow performance of read(). The specific problem is in the httplib.HTTPResponse constructor. It calls sock.makefile() with a 0 for the buffer size. changing the buffer size to 4096 improved the time needed to download 10MB from 15.5s to 1.78s, almost 9x faster. Repeat downloads of the same file (meaning the server now has the file cached in memory), yield times of 15.5s and 0.03s, a 500x improvement. When fetching from a server on the local network, rather than from localhost, these times become 15.5s and 0.9s in both cases, a 17x speedup. Real-world situations will likely be a mix of these, however it is safe to say the speed improvement will be substantial. Adding an option to adjust the buffer size would be very welcome, though the default value should still be zero, to avoid the issues already mentioned in issue 508157. These speed results were obtained with python2.5 and apache2 under Ubuntu linux, using the code found here: http://pastebin.ca/973578 |
|
|
| msg65127 - (view) |
Author: Daniel Diniz (ajaksu2) *  |
Date: 2008-04-07 21:52 |
| The code patch is trivial. I believe it needs docs (both explaining how to use and warning against the problems it may cause), a NEWS entry and tests (at least to check what happens when an invalid value lands). I can work on those changes if the general idea is not shot down :) |
|
|
| msg65199 - (view) |
Author: Facundo Batista (facundobatista) *  |
Date: 2008-04-08 17:12 |
| Daniel, Aren, please submit also what Daniel described, and I'll take a look and push it forward. Regards, |
|
|
| msg65232 - (view) |
Author: Daniel Diniz (ajaksu2) * |
Date: 2008-04-09 03:48 |
| "The code patch is trivial", he said, only to find out it was not :) Facundo, thanks in advance for taking a look at this! This patch tries to implement, document and test an optional argument to HTTPConnection, which passes it to HTTPResponse. So far, it's called "sockbuf", but I truly hope we can find a better name. The crux of the test is that it shouldn't be possible to use a buffered socket and a persistent connection simultaneously, as that was the reason a buffer was left out (see issue 508157). It also tries to check correctly allowing Keep-Alive when sockbuf==0. However, it fails to exercise HTTPResponse properly and needs a review down the auto-reconnect path. Persistent connections should be tested. Regarding the code, there's a chance that some changes touching forced closing are bogus (but not harmful). I'll get back to it and to persistent connection tests. Thanks again :) |
|
|
| msg65405 - (view) |
Author: Daniel Diniz (ajaksu2) * |
Date: 2008-04-12 16:53 |
| Also reported in #1542407 |
|
|
| msg91491 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2009-08-12 07:54 |
| I tried to use the following to change the buffersize for a download: from base64 import encodestring from httplib import HTTPResponse,HTTPConnection,HTTPSConnection,_UNKNOWN from datetime import datetime class FHTTPResponse(HTTPResponse): def __init__(self, sock, debuglevel=0, strict=0, method=None): print "creating response" self.fp = sock.makefile('rb',4096) self.debuglevel = debuglevel self.strict = strict self._method = method self.msg = None # from the Status-Line of the response self.version = _UNKNOWN # HTTP-Version self.status = _UNKNOWN # Status-Code self.reason = _UNKNOWN # Reason-Phrase self.chunked = _UNKNOWN # is "chunked" being used? self.chunk_left = _UNKNOWN # bytes left to read in current chunk self.length = _UNKNOWN # number of bytes left in response self.will_close = _UNKNOWN # conn will close at end of respons class FHTTPConnection(HTTPConnection): response_class = FHTTPResponse class FHTTPSConnection(HTTPSConnection): response_class = FHTTPResponse conn = FHTTPSConnection('localhost') headers = {} auth = 'Basic '+encodestring('usernmae:password').strip() headers['Authorization']= t = datetime.now() print t conn.request('GET','/somefile.zip',None,headers) print 'request:',datetime.now()-t response = conn.getresponse() print 'response:',datetime.now()-t data = response.read() print 'read:',datetime.now()-t ..however, I saw absolutely no change in download speed. Aren, I notice in your pastebin code that you do response.read(10485700) in a loop rather than just one response.read(), why is that? |
|
|
| msg91493 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-08-12 10:30 |
| I must admit I don't understand the conflict between buffering and pipelined requests. This is all sequential reading and the buffer should be transparent, shouldn't it? |
|
|
| msg91494 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2009-08-12 11:11 |
| Well, for me, buffer size doesn't appear to have made any difference... |
|
|
| msg91594 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2009-08-15 05:52 |
| Note that http://bugs.python.org/issue4879 may have already fixed this problem in trunk r68532. |
|
|
| msg91595 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2009-08-15 06:27 |
| Okay, I do not think this has been fixed yet. Anyone calling getresponse() can indeed use buffering=True, it can mess things up if the do not close the connection afterwards. The addition of the sockbuf parameter to HTTPConnection as proposed in buffered_socket.diff will work, but I'd follow the earlier work's lead. Don't expose it as the "sockbuf" integer. Just use a boolean "buffering" parameter that defaults to False. When true, do the same thing you do with sockbuf != 0. I see little value in actually being able to specify the exact buffer size used on the internal makefile on the HTTPConnection. The default will be sufficient. If you still want the user to be able to control it, perhaps add a HTTPConnection class attribute that defaults to -1 (socket.socket.makefile()'s bufsize default value) that you pass to makefile. Users can subclass HTTPConnection and give it a new value in that case. Personally I'd call that overkill. |
|
|
| msg91599 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2009-08-15 09:15 |
| Why not allow True or an integer as values for a buffer_size parameter to the HTTPConnection constructor. False would be the default, which would mean "no buffering" as currently is the case. True would mean use buffering of the default size and an integer value would mean use buffering of that size? Out of interest, has any of the proposed patching from this issue, [] or [] been marged to the 2.6 branch? PS: As I said, for me, changing the buffer size made no difference, so I may have to open up a separate issue once I figure out what's going on... |
|
|
| msg91617 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2009-08-15 22:02 |
| Anything that adds a new parameter can not be backported to 2.6 as that counts as an API change / feature addition. |
|
|
| msg91622 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2009-08-15 22:41 |
| trunk r74463 now forces the HTTPResponse with buffering=True to close afterwards using a HTTPResponse._must_close flag similar to what was suggested in buffered_socket.diff in this issue. |
|
|
| msg91625 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2009-08-15 23:00 |
| I am also unable to reproduce the reported problem using the pastebin.ca/973578 code. The time to download 400mb from localhost remains the same regardless of buffering=False (default) or True. The problem still exists but it is better described in and should only effect the performance of reading the HTTP headers (a lot if you're writing an application doing small/medium RPC requests over HTTP). |
|
|
| msg92246 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2009-09-04 11:18 |
| Yep, having done some more extensive profiling, it looks like my issue is different: all the time is being spent in httplib's HTTPResponse._read_chunked. That wouldn't be a symptom of this issue, would it? |
|
|
| msg124057 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-12-15 19:45 |
| This was apparently fixed in r69209. |
|
|