Lookahead buffer for ServeContent #1876
Conversation
acud
moved this from In progress
to In review (includes Documentation)
in Swarm Core - Sprint planning
api/http/server.go
Outdated
| case "buffered": | ||
| return langos.NewBufferedReadSeeker(s, getFileBufferSize) | ||
| case "langos": | ||
| return langos.NewBufferedLangos(s, getFileBufferSize) |
There was a problem hiding this comment.
We can use regular langos here (not buffered), also, since the peek tail is preserved. I do not get conclusive measurements that buffered one is faster at the current state.
There was a problem hiding this comment.
LGTM, coulf not find any issues with this PR, other than
- this solution seems to be still quantised: if one rogue chunk blocks the lookahead.
- travis shows https://travis-ci.org/ethersphere/swarm/jobs/602828430#L837 on TestLangosCallsPeekOnlyTwice
| // NewLangos bakes a new yummy langos that peeks | ||
| // on provided reader when its Read method is called. | ||
| // Argument peekSize defines the length of peeks. | ||
| func NewLangos(r Reader, peekSize int) *Langos { |
There was a problem hiding this comment.
The package is called langos and this constructor is also called NewLangos, so ultimately we end up with langos.NewLangos which is not very user-friendly unless you are intrinsically familiar with this code. LookaheadBufferedReadSeeker or smth like that would be much better than langos imo.
There was a problem hiding this comment.
Yes, Langos is used as a codename and is not so descriptive. We can rename it, but it would be nice to have something shorter. Since this is the only codename in the package with the same name as the package. Maybe a package doc comment that describes what langos stands for would help.
|
It would have been nice if the PR has a summary of the results of this change. Has this been tested in a real-world setting where you have 20ms-150ms roundtrip times to nodes, and some chunk reads are orders of magnitude slower than others? |
|
Thanks @nonsense for the review. I did perform a series of tests on public swarm network, but was able to do so only before I merged backward incompatible changes and protocol versions bump. Results that I had were up to 2x faster download speed with buffered Langos then by using BufferedReadSeeker. Unfortunately, that was still during the development and I did not record results. I was uploading files (from 2 to 50MB) to one of the swarm gateway nodes and dowloaded them locally. For that, I had to raise SearchTimeout drastically in order to be able to have successful downloads. Results with better connected nodes on a private cluster show much less performance gain. Average download speed on a standard smoke test jumps from 3.5Mb/s to 4.5MB/s and maximal download speed jump from 6MB/s to 8.5MB/s. Maximal download speed happens in frequent peeks. master (buffered read seeker): |
|
@janos great work, based on your results this is definitely an improvement. |
|
Thanks @nonsense. I expected more, but it is at least some improvement. |
This is a naive approach to implement a io.ReadSeeker with a lookahead buffer to be used with potentially slower reads from provider reader and for http.ServeContent.
fixes #1524