-
Notifications
You must be signed in to change notification settings - Fork 444
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
Data channels quickly slow down to a crawl under pressure #1088
Comments
OK, I had some midnight debugging, and I believe the main reason is incorrect reset of In the bit that refers to T3-rtx timer I believe this line above it is the cause: sipsorcery/src/net/SCTP/SctpDataSender.cs Line 573 in 5466285
Here T3-rtx timer is considered expired when RTO milliseconds passed since the last time the chunk was sent at. I believe this is wrong, because from my understanding of RFC T3-rtx timer is not the chunk property, but rather the destination property. I might be mistaken here, maybe The second bit that enters retransmit mode I believe simply used the wrong formula from RFC. It should have used Additionally, I believe in
I interpret this as |
some of the fixes are probably legit, but I believe the T3-rtx timer is not implemented correctly kinda works around sipsorcery-org#1088
some of the fixes are probably legit, but I believe the T3-rtx timer is not implemented correctly kinda works around sipsorcery-org#1088
@lostmsu, thank you for finding this out. I would like to have the patches applied to the one I am currently using. It says that the commits don't belong to a branch. Are the patches under the same license as SipSorcey? I also noticed that on a not-really-bad server, with dd if=/dev/zero bs=1k piped to it, the throughput is about 768kB/s (kilobytes) and the CPU usage is... not bad, but ok. I have no idea how to monkeypatch C# projects locally so it might take some time. Rust gives me around 2-3 MB/s, but due to some issues, I had to move from |
@ris-work same license If you want to experiment, ignore the first commit, I edited it and force-pushed as the second commit. You would need to get their branch and cherry-pick the commits from it (the branch has many breaking changes of various stability). |
Just for context the SCTP and data channels implementation was whipped together in a month to replace a previous native dependency. That was the last major effort I undertook on this library before starting a different venture which consumed all my time. Heaps of features are missing and no proper performance testing, let alone optimisations, have been carried out. @lostmsu I see there are lots of improvements in your #1192 PR. If it's feasible it'd be great if the data channel improvements could be put into a dedicated PR so they can be tested in isolation. |
Added to enhancements tracking issue. |
Please see #1087 for the test project.
P.S. I think a unit test for transmitting a few GiB of data should be put in place.
The text was updated successfully, but these errors were encountered: