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

Performance and bugfixes used by Borg #1192

Open
wants to merge 92 commits into
base: master
Choose a base branch
from

Conversation

lostmsu
Copy link
Contributor

@lostmsu lostmsu commented Oct 7, 2024

Uses BouncyCastle 2

Removes a good chunk of allocations and uses Span<byte> instead.

Parsed packets are no longer allocated either. Instead, they refer to a preallocated slice of some buffer, and their properties simply read data at corresponding offsets.

Rewrote logging in a few locations to use logging template strings (using C# string interpolation causes allocations and should never be used in critical performance paths).

Many other small performance fixes.

Some things to worry about

Drops support for .NET Core 3.1 and .NET 5.0 because they are no longer supported by MS. (could be reverted, but why?)
Also drops .NET Standard 2.1 because it could only really be used by the above two.

I had to hack datachannel flow control because the implementation SIPSorcery had would eventually slow to a crawl I assume due to a bug somewhere. I could not find that somewhere, so instead made some changes to flow control as seemed more appropriate. I remember doing fixes to something that I believe was not implemented according to the standard, but I believe I also made changes that have nothing to do with the standard.

Fixes

#1088
#1070
#1173
a few other data race issues due to lack of proper synchronization
maybe see the commit titles to get some more ideas

P.S. Potentially supersedes #1190
P.P.S. I made this available as https://www.nuget.org/packages/Borg.SIPSorcery/

camnewnham and others added 30 commits June 27, 2023 18:03
for some reason .1 did not include the changes o-O
@ValorZard
Copy link

👀 This looks really dope! I was JUST looking into this library for using its implementation of datachannels! Hopefully this will make this library as performant as something like Pion or lib-datachannels

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 8, 2024

Sadly not there. I peak at 30MB/s while Pion is able to do full 1Gbps (e.g. ~100MB/s).

Funnily enough, Rust implementation has the same problem. I don't believe we are hitting the performance ceiling of .NET here. I think the flow control implementation is incorrect.

@ValorZard
Copy link

Yeah, i mean Go and C# are about equivalent performance wise (I wouldn't be surprised if .NET is actually faster). Maybe there's some cool stuff Go is doing with Goroutines?

@ha-ves
Copy link
Contributor

ha-ves commented Oct 8, 2024

Drops support for .NET Core 3.1 and .NET 5.0 because they are no longer supported by MS. (could be reverted, but why?)
Also drops .NET Standard 2.1 because it could only really be used by the above two.

@lostmsu

I guess it's time this library branches into specific .net versions(?)

I think that's also what's been holding back the performance(?)

Which part would not be implemented if you still supported those versions?

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 8, 2024

There's really no part of the library that would not work on these targets. But because they are out of support having them is mostly waste of resources.

@ha-ves
Copy link
Contributor

ha-ves commented Oct 9, 2024

Unless there's a significant performance hit when having legacy support,
I feel like ending legacy support because of nothing is the waste of resources.

@sipsorcery
Copy link
Member

Thanks for the PR. It's a big chunk of work!

I used the Borg.SIPSorcery nuget package for the client side of the tests here and got 4 failures with the servers below:

  • kurento
  • libdatachannel
  • gstreamer
  • janus

aiortc also fails but that's currently happening with the main SIPSorcery v8.0.0 package due to ECDSA ciphersuites only being offered in the DTLS handshake for some reason.

I suspect the failures are mostly related to the BouncyCastle update. Every other time that has been attempted DTLS breaks.

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 15, 2024

@sipsorcery can you help me build one of these? I'm on Windows, not sure where to do vcpkg install --triplet=x64-windows openssl zlib for libdatachannel. It seems to assume I have a vcpkg project (vcpkg.json) 1 level above.

Or tell me if something is easier to build starting off just Visual Studio 2022.

@sipsorcery
Copy link
Member

sipsorcery commented Oct 15, 2024

I wouldn't recommend attempting to build them manually unless you have a reason to do so (and note some, like libwebrtc, can take weeks to succeed).

Instead you can jsut use the prebuilt docker images. They're hosted in a public container registry. For libdatachannel just run:

docker run -it --rm --init -p 8080:8080 ghcr.io/sipsorcery/libdatachannel-webrtc-echo

If it works you'll have the a WebRTC peer sitting there waiting for a connection.

Easiest way to test is then to just use dotnet run in the sipsorcery client directory.

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 15, 2024

@sipsorcery should probably mention prebuilt container in the README of echoes

@sipsorcery
Copy link
Member

@sipsorcery should probably mention prebuilt container in the README of echoes

It is... Always has been.

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 16, 2024

@sipsorcery damn I am sorry, bad day )

@ris-work
Copy link

@lostmsu: Thank you for the fixes; I can confirm that they have made things far more performant than without it! However, I had to revert it due to the PR #1140 because of some BouncyCastle incompatibility...

@lostmsu
Copy link
Contributor Author

lostmsu commented Oct 19, 2024

@ris-work dang, thanks for notifying.

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.

6 participants