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

Fix data-channels-flow-control example #846

Open
hugoArregui opened this issue Sep 25, 2019 · 5 comments
Open

Fix data-channels-flow-control example #846

hugoArregui opened this issue Sep 25, 2019 · 5 comments

Comments

@hugoArregui
Copy link
Member

hugoArregui commented Sep 25, 2019

There is a problem with the current example, the peer may stop sending data here if the max buffer size has not been reached before this point and since the callback is executed as part of the stream processing thread, that will block everything.

So we need to:

  1. Find a better flow control mechanism for the example
  2. Decide if we should execute the callback in another goroutine so we ensure we don't block the stream, or, document the fact that users should make sure their code don't block.

As a suggestion for (1) I wrote this code: https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L31 which uses a queue and drains it whenever it's possible. It might not the most efficient way having a channel in the middle of the write operation but I like how easy is to reason about it. I think it makes sense for an example.

Regarding (2) I think we should probably execute the callback in another goroutine since that's consistent with do things in other places, but I have no strong opinion on this matter.

@hugoArregui
Copy link
Member Author

CC @enobufs

@AeroNotix
Copy link
Contributor

How have you tested the example (suggestion 1)? I have tried it myself and found a few issues. I am still isolating my own code from that example but I would like to see how you tested it to verify for correctness?

@hugoArregui
Copy link
Member Author

@AeroNotix I ran it with a 1.5gb file (uncommenting https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L138 and adding a loop when I wanted more data) and different combinations of max buffer size / queue size / threshold. In this particular case please notice the channel is unreliable (https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L120), so if you want to try this very same example you may want to set some max retransmits.

@AeroNotix
Copy link
Contributor

It would be interesting to have a runnable example, alone, that you specify what parameters it is supposed to exhibit - then I run that - and I can confirm or deny that it exhibits the same or similar performance.

Without that we are pissing into the dark, with the wind at our face.

@AeroNotix
Copy link
Contributor

AeroNotix commented Sep 26, 2019

E.g. I would be interested in these metrics, from your example:

  • How did it perform when running between remote peers. Running networking benchmarks against localhost is fine to get a baseline for potential performance but running against remote peers can allow to find timing issues, or bottlenecks that localhost<>localhost transfers may not expose.
  • When running between remote peers - is all data actually transferred?

@Sean-Der Sean-Der added this to the 3.2.0 milestone Jan 23, 2022
@Sean-Der Sean-Der removed this from the 3.2.0 milestone May 22, 2022
@Sean-Der Sean-Der removed the triaged label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants