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

use a new stream per request #167

Closed
wants to merge 2 commits into from
Closed

use a new stream per request #167

wants to merge 2 commits into from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jun 20, 2018

"pipelining is hard, painful, and error-prone" is a key reason browsers gave up on it and started implementing stream muxing.

Guess what, we have stream muxing!

Now, this is what we used to do (i.e., we used to use one stream per request).

There are two reasons not to:

  1. Setting up a stream is expensive. It's really not. Yes, we have to send a bunch of extra bytes but it usually doesn't even cost a round trip (we should have a guaranteed no round trip but, well, we don't).
  2. No backpressure. We can and should fix this at a lower level (this should be a host<->host negotiation).

In terms of performance, I had to remove the lower latency bound in the test cases to get them to pass.

@ghost ghost assigned Stebalien Jun 20, 2018
@ghost ghost added the status/in-progress In progress label Jun 20, 2018
@Stebalien
Copy link
Member Author

Note: This PR is intentionally inflammatory and exists for discussion.

@whyrusleeping
Copy link
Contributor

show me the numbers

@Stebalien
Copy link
Member Author

Stebalien commented Jun 25, 2018

One stream per request:

Parallel PutValue calls (100) completed in 2.726807896s
Parallel GetValue calls (100) completed in 2.38722302s
Sequential PutValue calls (10) completed in 10.10373804s
Sequential PutValue calls (10) completed in 4.897108013sh

Pipelined:

Parallel PutValue calls (100) completed in 6.155222485s
Parallel GetValue calls (100) completed in 4.461936237s
Sequential PutValue calls (10) completed in 10.477949103s
Sequential GetValue calls (10) completed in 4.522981736s

Note: the timing can vary by up to a second. However, this patch is definitely faster in the mock net.

However, I don't know why we switched away from using one stream per request in the first place.

@whyrusleeping
Copy link
Contributor

And bandwidth usage? (as tracked by the OS)

@Stebalien
Copy link
Member Author

30%? WTF multistream select!


One of my plans for the libp2p meetup is to improve multistream-select. I'll add "low overhead" to the list. Ideally, identify would negotiate connection specific aliases that are just varints.

@Stebalien Stebalien closed this Jun 25, 2018
@ghost ghost removed the status/in-progress In progress label Jun 25, 2018
@whyrusleeping
Copy link
Contributor

Yeah... this is why I did the stream reuse stuff in the first place. It makes me sad. Hear that @jbenet ?? SAD

@anacrolix
Copy link
Contributor

Why was this closed? Was it due to extra bandwidth requirements?

@anacrolix anacrolix mentioned this pull request Feb 20, 2019
@Stebalien
Copy link
Member Author

Yes.

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.

3 participants