-
Notifications
You must be signed in to change notification settings - Fork 292
Update WebRTC message size limit #628
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
Conversation
The browser bugs that made a 16KiB message size limit necessary have long been fixed. Increase the max size to 256KiB which has been verified experimentally between Firefox/Chrome and update the link to the Mozilla blog entry that discusses the limit and the relevant WebRTC issue that enables 256KiB messsages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we account for forwards compatibility with future implementations that aren't arbitrarily limited like browsers currently?
A "be liberal in what you accept, conservative in what you emit" sort of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer separate sentences since they're both important and about different things.
Implementations MAY choose to send smaller messages, e.g. to reduce delays
sending _flagged_ messages.
Implementations MAY choose to accept larger messages
to allow for forwards compatibility with more capable implementations in the
future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on internal discussions, I believe we've decided to keep it at 16KiB to remain compatible with Kubo 0.30.0.
@achingbrain what is the next step here? figure out next steps without breaking interop? (e.g. how to negotiate higher size limit per connection via something like RTCSctpTransport/maxMessageSize?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m doing some experiments in the test plans repo to validate that we do in fact see a throughput increase with larger messages.
Marking this as a draft pending the outcome of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See linked performance graphs here: #628 (comment)
2color
commented
Nov 1, 2024
I'm curious, what was the conclusion with this PR and the potential throughput increase?
|
The result was a roughly 30% speed increase when going from 16KB to 265KB chunks. See "Throughput" at the bottom of this page: https://observablehq.com/@libp2p-workspace/performance-dashboard?branch=a44f101803381ea2547e2eb44abc7fb6279d41f2#branch
I think this is still worth doing but we need the go/rust implementations to accept larger chunks - I think rust-libp2p will still reject them. |
p-shahi
commented
Dec 2, 2024
Here's the draft pr @sukunrt created for go-libp2p: libp2p/go-libp2p#2949
The rust-libp2p pr is waiting for the spec to be merged before shipping: libp2p/rust-libp2p#5589
Is everyone agreed that we want to increase the message size? If so, we can proceed with merging the go & rust pr
@achingbrain @sukunrt
@p-shahi I think we should increase the message size. WebRTC is by far the slowest libp2p transport but also one of the most useful since it's the only one that lets browsers dial each other directly, so anything that speeds it up is very welcome.
@sukunrt
sukunrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a way to make this backwards compatible for /webrtc-direct using the datachannel 0 that we use for handshaking.
-
Both server and client use SDPs that indicate that peer supports 256kB messages.
-
sever and client use the noise handshake early data extension to send the max message size.
-
Depending on the max message size received in the handshake we the server and client decide whether to actually send 256kB messages or 16 kB messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you edit the webrtc-direct file to remove this line where we recommend a max-message size of 16kB
https://github.com/libp2p/specs/blob/1253e2a1deec908037fba924fcd350ec16df13a6/webrtc/webrtc-direct.md?plain=1#L79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 256kB, can we support more? Can we add links for the browser specific limts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer separate sentences since they're both important and about different things.
Implementations MAY choose to send smaller messages, e.g. to reduce delays
sending _flagged_ messages.
Implementations MAY choose to accept larger messages
to allow for forwards compatibility with more capable implementations in the
future.
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This also forces a change to the connection SCTP read buffer to be increased to about 2.5 MB, to support 1 message being buffered for 10 streams. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This also forces a change to the connection SCTP read buffer to be increased to about 2.5 MB, to support 1 message being buffered for 10 streams. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This also forces a change to the connection SCTP read buffer to be increased to about 2.5 MB, to support 1 message being buffered for 10 streams. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This also forces a change to the connection SCTP read buffer to be increased to about 2.5 MB, to support 1 message being buffered for 10 streams. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
In experiments with js we've found that increasing the message size increases throughput. See: libp2p/specs#628 (comment) for details. This changes the protobuf reader for the stream to read 256kB messages. This also forces a change to the connection SCTP read buffer to be increased to about 2.5 MB, to support 1 message being buffered for 10 streams. This isn't enough to support larger messages. We most likely need to change the inferred SDP of the server to use 256kB maxMessageSize, and need some backwards compatible mechanism in the handshake to opt in to large messages. See: libp2p/specs#628 for details
Uh oh!
There was an error while loading. Please reload this page.
The browser bugs that made a 16KiB message size limit necessary have long been fixed.
Increase the max size to 256KiB which has been verified experimentally between Firefox/Chrome and update the link to the Mozilla blog entry that discusses the limit and the relevant WebRTC issue that enables 256KiB messages.
Implementation status