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

Update WebRTC message size limit #628

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Sep 2, 2024

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

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.
@2color 2color mentioned this pull request Sep 3, 2024
32 tasks
Comment on lines 94 to 95
Implementations MAY choose to send smaller messages, e.g. to reduce delays
sending _flagged_ messages.
Copy link
Member Author

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?

Suggested change
Implementations MAY choose to send smaller messages, e.g. to reduce delays
sending _flagged_ messages.
Implementations MAY choose to send smaller messages, e.g. to reduce delays
sending _flagged_ messages, and they MAY choose to accept larger messages
to allow for forwards compatibility with more capable implementations in the
future.

A "be liberal in what you accept, conservative in what you emit" sort of thing.

Comment on lines +91 to +93
Encoded messages including their length prefix MUST NOT exceed 256kiB to support
all major browsers. See ["Large Data Channel Messages"](https://blog.mozilla.org/webrtc/large-data-channel-messages/)
and [WebRTC#40644524](https://issues.webrtc.org/issues/40644524).
Copy link
Member

@lidel lidel Sep 5, 2024

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?)

Copy link
Member Author

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.

@achingbrain achingbrain marked this pull request as draft September 5, 2024 18:08
@2color
Copy link
Contributor

2color commented Nov 1, 2024

I'm curious, what was the conclusion with this PR and the potential throughput increase?

@achingbrain
Copy link
Member Author

achingbrain commented Nov 1, 2024

The result was a roughly 15-25% 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=feat%2Fadd-circuit-relay#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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

4 participants