-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Streams optimization #496
Streams optimization #496
Conversation
Sure! Please re-write readline(), do not touch tests, I'd like to test it on real system |
@fafhrd91 Done. In my opinion |
Thank you a lot @homm, I'm pretty excited to test that with the FrameworkBenchmarks suite to see the difference with the previous version of aiohttp. Can I already test with this branch, or I must wait that you fix all the tests compliance first ? |
@GMLudo should be fine without tests |
@homm Nice work! In additional to aiohttp test, it also breaks aiocouchdb ones:
I tried to reproduce the problem with public instance, but here I also found that patch breaks multipart reader as well in other way: import asyncio
import aiohttp
@asyncio.coroutine
def go():
resp = yield from aiohttp.request(
'get',
'https://kxepal.cloudant.com/aiohttp-496-bug/docid',
headers={'Accept': 'multipart/related'},
params={'attachments': 'true'})
reader = aiohttp.MultipartReader.from_response(resp)
while True:
part = yield from reader.next()
if part is None:
break
print((yield from part.read()))
if __name__ == '__main__':
asyncio.get_event_loop().run_until_complete(go()) Expected:
Actual:
while it should stop right after '\r\n\r\n'. Thoughts? If you need any additional information, feel free to ask. |
@kxepal Thanks, I've fixed the error in But I can't reproduce the error with
But except for these errors all tests are passed.
|
@homm Oh, my. It seems I didn't push the fixes. Thanks for notice! The problem happens with Sorry for false alarm (: |
Unfortunately no, because there is also I had the idea to create a wrapper around |
@homm As for idea to mimic deque to bytesarray, I think it doesn't worth any effort. In the end, field is private - there are no guarantees provided on them. |
After that I have various exceptions from new master:
|
@homm hm...what's your fd limit? |
@homm i tested your patch in production, everything is good. we need to fix kxepal's tracebacks and update tests, then lets merge change. regarding performance, in system with normal load, small/fast requests, mostly requests to external subsystems difference is not visible. but that is expected. in system with a lot of load (file upload/download) performance slightly better, but memory usage is much better. |
ping |
Sorry. I'll try to find some time for this tomorrow. |
Ok, done. Instead of checking internal |
\o/ it's green! +1 |
Thanks! |
This is proof of concept of significant
StreamReader
speedup. The main idea is avoiding memory copying and reallocations. Thecollections.deque
is used instead of thebytearray
, but unlike forDataQueue
, no reading limitations are applied.Performance
My benchmarks: https://gist.github.com/homm/3b9c2909905f76668bf9
Memory
As a result of removing relocations, the memory consumption and fragmentation is also greatly reduced. I've got following result for the benchmark:
test_stream_read_all
test.test_stream_read_all
test.Distinct
The main difference is how
read(BIG_LIMIT)
andreadany()
produce the result when the buffer is full: In the old implementation, the whole buffer is returned at once. In the new implementation, only the first chunk is returned.