Skip to content
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

Limited read buffer size in Async backend to 32K. #330

Merged
merged 1 commit into from
Apr 22, 2015

Conversation

artemkin
Copy link
Contributor

There was unlimited buffer allocation in Async backend. It affected uploading of big files a lot. It was at least 10 times slower than downloading the same file. After the fix, uploading and downloading speeds seem to be on par.

Anyway, it is still not that good as new buffer is allocated for every single chunk. The buffer should be reused for all chunks for better performance.

@rgrinberg
Copy link
Member

Yes the buffer allocation is very much not ideal. There's no "easy" way to fix it either. The whole IO layer should be reworked to stop allocating stuff with more efficient buffer management reimplemented on top of that.

Your fix is good however as we really should have a limit on the buffer size that we allocate. I'm merging this in.

Thanks.

@artemkin
Copy link
Contributor Author

Yes, I see. Maybe add it to #198 or report a separate issue? Performance is a feature for production systems. It seems people (including myself) wish to use cohttp for real world tasks (see #328).

I would contribute more, but the biggest problem for me is actually reading the codebase. A lot of functors + module openings just blow my mind. Any tips how to read that?

rgrinberg added a commit that referenced this pull request Apr 22, 2015
Limited read buffer size in Async backend to 32K.
@rgrinberg rgrinberg merged commit f83a47e into mirage:master Apr 22, 2015
@rgrinberg
Copy link
Member

#198 only talks about interface breaking changes. I would hope that sane buffer management is not going to break people's code (but I've been wrong about this before).

I'll try and write something up on how to contribute/understand cohttp a little more. We do end up with a lot of complexity for trying to support multiple backends. But there's method to this madness.

@avsm
Copy link
Member

avsm commented Apr 23, 2015

@artemkin I feel your pain. One good way is to work from the lib/ directory into async/ and then into lwt/. The Merlin IDE tool helps unwrap the types. Documentation would most definitely help though.

@trevorsummerssmith
Copy link
Contributor

@avsm Getting off topic now but I think the high level design you wrote in this thread: http://lists.xenproject.org/archives/html/mirageos-devel/2015-03/msg00137.html would be very helpful to drop into the codebase basically verbatim.

@artemkin
Copy link
Contributor Author

@rgrinberg @avsm Could you release v0.17.1 with this fix? It is critical defect, and it is not convenient to use pinned repo. Guys who admin the server wish to build it locally, and they're not OCaml-ers yet)

@avsm
Copy link
Member

avsm commented Apr 24, 2015

Yes; can do this afternoon.

@avsm
Copy link
Member

avsm commented Apr 24, 2015

See #332 and DESIGN.md skeleton committed, @trevorsummerssmith

@trevorsummerssmith
Copy link
Contributor

Thanks I think that will be a great help to new contributors!

@artemkin
Copy link
Contributor Author

Great! Thank you

@vbmithr
Copy link
Member

vbmithr commented May 29, 2015

Please revert this. The semantics of read / read_exactly is not conserved anymore and this introduce bugs (Like #357).

@mseri mseri mentioned this pull request Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants