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

dynamic requests pipeline length #190

Merged
merged 1 commit into from Nov 27, 2014
Merged

Conversation

@astro
Copy link
Contributor

astro commented Nov 27, 2014

With the fixed MAX_OUTSTANDING_REQUESTS we're running into the following problems when peer communication suffers from delay:

  • High-bandwidth connections become stop'n'go because there are no queued requests while the previous requests' data is still in-flight.
  • Low-bandwidth peers will have requests queued that will arrive much in the future, affording for the intelligent piece selection that is approached in Torrent.prototype._updateWire(). Unfortunately, this mechanism is difficult to understand, lacks documentation, and has multiple TODOs. I was observing quite a delay when seeking during streaming playback.

I propose using a pipeline length based on a bandwidth-delay-product. Because we cannot measure delay properly in pipelined TCP communication, we assume a constant of half a second. Large enough to avoid jitter on TCP packet loss for most links, small enough to retain agility when reselecting (seeking in streams). There's a lower limit (PIPELINE_MIN_DURATION) and an upper limit (PIPELINE_MAX_DURATION) to send requests in batches, thereby making use of the Nagle algorithm.

By confining the pipeline length in a time-based way I think you could throw away a lot of calculation in speedRanker(). Of course there's a downside with high-latency links, but it will still work with the minimum of 3 in-flight requests.

The terminology is arbitrary, feel free to rename getPipelineLength() to something else.

@@ -650,6 +652,10 @@ Torrent.prototype._updateWire = function (wire) {
if (wire.peerChoking) return
if (!wire.downloaded) return validateWire()

var minOutstandingRequests = getPipelineLength(wire, PIPELINE_MIN_DURATION)
if (wire.requests.length >= minOutstandingRequests) return true

This comment has been minimized.

Copy link
@feross

feross Nov 27, 2014

Member

This should just be return instead of return true since the return value is not used. Please confirm.

This comment has been minimized.

Copy link
@astro

astro Nov 27, 2014

Author Contributor

Of course. I got confused because of the return validateWire() above and because one of the subfunctions returns a boolean.

@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 27, 2014

I am in favor of documenting, refactoring, or removing the _updateWire function and replacing it with something simpler. In fact, the whole torrent.js file could use some love. I was planning to do this at the end of December (when I'll finally have some free time), but if you're interested in having a crack at it sooner, then by all means please do. It's important work to make webtorrent streaming more reliable.

All your changes make sense to me (except for my one inline comment), so let's merge. :)

@feross feross merged commit 566b47c into webtorrent:master Nov 27, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@feross

This comment has been minimized.

Copy link
Member

feross commented Nov 27, 2014

Released as 0.15.0.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.