classification
Title: Enhance HTTPResponse.readline() performance
Type: performance Stage: commit review
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: kristjan.jonsson, orsenthil, pitrou, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-09-13 13:39 by kristjan.jonsson, last changed 2014-03-19 16:07 by kristjan.jonsson. This issue is now closed.

Files
File name Uploaded Description Edit
peek.patch kristjan.jonsson, 2013-09-13 13:39 review
httpresponse.patch kristjan.jonsson, 2013-09-16 22:29 review
httpresponse.patch kristjan.jonsson, 2013-09-20 13:58 review
httpresponse.patch kristjan.jonsson, 2013-09-21 16:43 review
httpresponse.patch kristjan.jonsson, 2013-09-27 11:53 review
Messages (24)
msg197574 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-13 13:39
Some applications require reading http response data in "long" polls as it becomes available.  This is used, e.g. to receive "notifications" over a HTTP stream.
Using response.read(large_buffer) is not possible because this will attempt to fullfill the entire request (using multiple underlying recv() calls), negating the attempt to get data as it becomes available.
Using "readline", and using \n boundaries in the data, this problem can be overcome.
Currently, readline is slow because it will attempt to read one byte at a time.  Even if it is doing so from a buffered stream, it is still doing it one character at a time.
This patch adds a "peek" method to HttpResponse, which works both for chunked and non-chunked transfer encoding, thus improving the performance of readline.  IOBase.readline will use peek() to determine the readahead it can use, instead of one byte which it must use by default.
msg197587 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-13 15:16
This sounds ok on the principle. I suppose one can't simply wrap the "fp" inside a BufferedReader?
I think it would be good to add tests for the peek() implementation, though.
msg197595 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-13 15:35
The problem is that self.fp is already a Buffered stream, and such streams are documented to have their read() and readinto() calls make multiple system calls to fullfill the request.

My original goal was actually to make response.read(amt) not try to make multiple read() calls, so that one could have other delimiters than newline.  It is simple for the chunked case, but I don't know how to bypass it for response.fp, since it is already a buffered file that has no guaranteed such behaviour.... wait, one can simply call peek() on it and know how much one can get :)

But anyway, the use case I have actually uses newlines as record separators in the http stream, so this enhancement seems less intrusive.

I'll add unit tests.  There ought to be ready-made HTTPResponse tests already that I can use as templates.
msg197598 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-13 15:37
> My original goal was actually to make response.read(amt) not try to
> make multiple read() calls, so that one could have other delimiters
> than newline.  It is simple for the chunked case, but I don't know
> how to bypass it for response.fp, since it is already a buffered
> file that has no guaranteed such behaviour.... wait, one can simply
> call peek() on it and know how much one can get :)

Or you can use read1()?
msg197601 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-13 15:48
See also issue 13541?
msg197603 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-13 15:50
Intersting. I didn't know about that.  My excuse is that I never use 3.x except when I'm porting some CCP enhancements for cpython.

Here's a thought:  HTTPResponse inherits from RawIOBase.  Only the BufferedIO classes have read1() and are documented to have more than one system calls on their read() methods.  Yet, HTTPResponse will happily behave in that way.
So, either HTTPResponse, being a non-buffered IO object, should not work in that way, or we could add a read1() method to it.
msg197604 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-13 15:53
I should add, I fully support the use case that response.read(amt=None) needs to read to the end of the response.  It is only the read(amt=bufsize) use case I'm thinking of, and that could be handled with a read1() method.
msg197950 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-16 22:29
Ok, I'm adding a more comprehensive patch.  It adds support for
peek, readline, and read1 for both regular and chunked responses.
readline falls back to IOBase.readline for chunked, which again makes use of peek() for performance.
read1() is available for other kinds of client buffering, e.g. when reading the stream incrementally using other delimiters than newline.

The real work is done by the _read1_or_peek_chunked() method.  the code for both cases is almost identical so a dual purpose method is warranted.

Added test cases to test this.  The test cases don't attempt to test that we don't block on partial resopnses, though.  I'm not sure how I would write such a test.  I'd probably have to extend FakeSocket to work on some stream that can be appended to...
msg198142 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-20 12:04
After adding read1() and peek() what stop us from inheriting HTTPResponse from BufferedIOBase?

I suggest split _read1_or_peek_chunked() by two parts. First part calculates n bounded by chunk_left (it can read the next chunk size and close a connection if needed). Perhaps it can be reused in _readall_chunked() and _readinto_chunked(() too. Second part which is  controlled by boolean parameter should be moved out in read1() and peek().
msg198148 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-20 13:58
Ok, I have refactored this a bit.
A separate new function now takes care of the reading of chunk-header and tail.  This simplifies the other functions.

I'm not sure what you mean by inheriting from the buffered class.  Do we gain anything by doing that, would it change the code? or would it merely be for the benefit of ABC?

Note that the chunk protocol was wrong and I fixed the unittests:  The final chunk is a _valid_ zero length chunk, i.e. 0\r\n\r\n.  It contains two eol tokens like other chunks, one at the end of the length, the other at the end of the(null) payload.
msg198157 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-20 18:39
> A separate new function now takes care of the reading of chunk-header and tail.  This simplifies the other functions.

Good. It is even better than I expected.

> Do we gain anything by doing that, would it change the code? or would it merely be for the benefit of ABC?

Yes, it for the benefit of ABC. Some code tests isinstance(file, io.BufferedIOBase) and wraps stream in BufferedReader if it is false.

> Note that the chunk protocol was wrong and I fixed the unittests:  The final chunk is a _valid_ zero length chunk, i.e. 0\r\n\r\n.  It contains two eol tokens like other chunks, one at the end of the length, the other at the end of the(null) payload.

How new code processes invalid final chunk 0\r\n? We need test to check that there is no degradation.

Perhaps it relates to behavior change which I mention in the comment on Rietveld.
msg198163 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-20 20:12
Ok, I can change the base class inheritance and see if it changes things.

Note for the different interpretation of the final chunk:
Chunked encoding allows for adding headers after the final chunk.  This is what _read_and_discard_trailer() does, but discarding the trailing headers.  So, if support is ever added for reading those trailing headers, then we need make sure that we consume the last chunk correctly.
It is, of course, a matter of resilience, v.s. correctness, but the lesson of Internet Explorer should tell us to prefer correctness over flexibility, I think.  
We can, of course, change this back. if you think.  It is ieasier now, that the chunk consumer is in a single method.
msg198166 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-20 20:21
> Note for the different interpretation of the final chunk:
> Chunked encoding allows for adding headers after the final chunk.
> This is what _read_and_discard_trailer() does, but discarding the
> trailing headers.  So, if support is ever added for reading those
> trailing headers, then we need make sure that we consume the last
> chunk correctly.

I think what Serhiy is proposing is that we also succeed when the
server sends a truncated last chunk (and I agree with him).
msg198171 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-20 21:08
Ok, I can make it resilient.  I was just pointing out that resilience in the face of RFC violations can be a bad thing.  E.g. Internet explorer and how they allowed internet servers of the world to be lax in how they served their data.
No matter, I can allow truncated tails when reading chunks.  But I don't think we ought to make that a requirement or make special unit tests for that.  Either you read chunked encoding correctly or you don't.  There are so many different ways you can break protocol, that this particular one is really only a special case.
msg198172 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-20 21:11
> Ok, I can make it resilient.  I was just pointing out that resilience
> in the face of RFC violations can be a bad thing.  E.g. Internet
> explorer and how they allowed internet servers of the world to be lax
> in how they served their data.

I'm afraid the ship has sailed a long time ago on that. Poor HTTP
implementations are everywhere.

> No matter, I can allow truncated tails when reading chunks.  But I
> don't think we ought to make that a requirement or make special unit
> tests for that.

Well, the unit tests already exist, which is why I think we shouldn't
stop supporting that.
msg198173 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-20 21:17
Very well, let's support both then :)
msg198212 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-21 16:43
Ok, Here is a new patch.
We now inherit from BufferedIOBase.  We must implement read(amt) ourselves, since the base class does not do it.

I was wrong about the final chunk, it is in fact 0\r\n.  A final \r\n is then added to signal the end of the optional trailers.

Added a bunch of tests for chunked encoding, plus tests to verify http synchronization for keepalive connections.
msg198446 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-26 16:53
Kristján, did you notice my comments on Rietveld?
msg198447 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-26 17:53
Ah no actually.  Thanks . I'll respond soon.
msg198469 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-09-27 11:53
A new patch set, in response to Serhiy's comments on Rietveld.
msg213963 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-03-18 14:09
LGTM.
msg214071 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-19 10:29
New changeset 49017c391564 by Kristján Valur Jónsson in branch 'default':
Issue #19009
http://hg.python.org/cpython/rev/49017c391564
msg214107 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-19 16:00
If this is committed, should the issue be closed?
msg214108 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-03-19 16:07
Sure.  If there are issues we'll just reopen.  Closing.
History
Date User Action Args
2014-03-19 16:07:11kristjan.jonssonsetstatus: open -> closed
resolution: fixed
messages: + msg214108
2014-03-19 16:00:31pitrousetmessages: + msg214107
2014-03-19 10:29:51python-devsetnosy: + python-dev
messages: + msg214071
2014-03-18 14:09:16serhiy.storchakasetstage: patch review -> commit review
messages: + msg213963
versions: + Python 3.5, - Python 3.4
2013-09-27 11:53:48kristjan.jonssonsetfiles: + httpresponse.patch

messages: + msg198469
2013-09-26 17:53:19kristjan.jonssonsetmessages: + msg198447
2013-09-26 16:53:58serhiy.storchakasetmessages: + msg198446
2013-09-21 16:43:48kristjan.jonssonsetfiles: + httpresponse.patch

messages: + msg198212
2013-09-20 21:17:18kristjan.jonssonsetmessages: + msg198173
2013-09-20 21:11:33pitrousetmessages: + msg198172
2013-09-20 21:08:45kristjan.jonssonsetmessages: + msg198171
2013-09-20 20:21:01pitrousetmessages: + msg198166
2013-09-20 20:12:31kristjan.jonssonsetmessages: + msg198163
2013-09-20 18:39:02serhiy.storchakasetmessages: + msg198157
2013-09-20 13:58:42kristjan.jonssonsetfiles: + httpresponse.patch

messages: + msg198148
2013-09-20 12:04:53serhiy.storchakasetmessages: + msg198142
2013-09-16 22:29:01kristjan.jonssonsetfiles: + httpresponse.patch

messages: + msg197950
2013-09-13 15:53:37kristjan.jonssonsetmessages: + msg197604
2013-09-13 15:50:47kristjan.jonssonsetmessages: + msg197603
2013-09-13 15:48:20r.david.murraysetnosy: + r.david.murray
messages: + msg197601
2013-09-13 15:37:51pitrousetmessages: + msg197598
2013-09-13 15:35:14kristjan.jonssonsetmessages: + msg197595
2013-09-13 15:16:58pitrousetversions: + Python 3.4, - Python 3.5
nosy: + serhiy.storchaka, orsenthil, pitrou

messages: + msg197587

stage: patch review
2013-09-13 13:39:30kristjan.jonssoncreate