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

perf: migrate from simple-peer to polite-peer #1

Closed
ThaUnknown opened this issue Jul 30, 2022 · 20 comments
Closed

perf: migrate from simple-peer to polite-peer #1

ThaUnknown opened this issue Jul 30, 2022 · 20 comments

Comments

@ThaUnknown
Copy link
Contributor

Please consider migrating from simple-peer to polite-peer: https://github.com/jimmywarting/jimmy.warting.se/blob/master/packages/peer/perfect-negotiation.js [there isn't an NPM package for this but I could make one]

Why this matters:
simple-peer relies on stream which in turn relies on buffer, and process which relies on setTimeout. This is an important issue because those require to be polyfilled, and those polyfills are VERY slow. Since WebTorrent was mentioned, from my testing more than half of the app's CPU time was consumed by node's stream and buffer or actually it's polyfills. Another massive issue is the reliance on the slow process polyfill of most bundlers which uses setTimeouts for delying tasks instead of promises or microtask.

Problems:
polite-peer is a much simper package than simple-peer [oh the irony] and as such doesn't expose that many wrappers so it might require extra work. Another issue might be the immaturity of the package as very little people actually used it [not to be confused with it wasn't used a lot, because it was used in public projects a lot] so unexpected issues might arise.

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented Jul 30, 2022

I forgot to explain why using timeouts instead of microtasks is bad: this is because when the browser tab in background all timeouts are capped at [at least] 4ms, this means your 1µs operation all of a sudden becomes 4ms which slows down the operation x4000!

This was referenced Jul 30, 2022
@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Thanks for this info - I learned a lot from this summary. I actually had this written using raw RTPPeerConnections and then just pulled in simple-peer to make those have a nicer API (but wasn't thrilled about the bloat) so I'll look into migrating to polite peer.

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Ah polite-peer is just packing in the "perfect negotiation" pattern, I had come across this code before actually, it's quite nice. However, I don't think it's going to work (or add much benefit) here since the negotiation pattern for P2PCF isn't using this approach but a bespoke one that takes advantage of the signalling server's custom protocol. Additionally, polite-peer doesn't have any of the bells and whistles for managing media streams and renegotiation that simple-peer does. Maybe there's another library that has similar features but less bloat + avoids the perf issue you mention?

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Also, I haven't dug into it - but I think the way you'd incur this performance tax is if you ended up using the Stream-like APIs with simple-peer when using P2PCF. I'd have to check to see if that code path gets exercised during the general send function, in which case if it doesn't at least for most data channel based apps doing simple messaging there won't be a tax. I'm guessing they use the stream API in WebTorrent for sending large files between peers.

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

In general it does seem like this issue would be best addressed by a PR to simple-peer, it’s a pretty nice library afaict and has a lot of downstream consumers that would benefit from fixing the issue you mention.

@ThaUnknown
Copy link
Contributor Author

Additionally, polite-peer doesn't have any of the bells and whistles for managing media streams

it exposes the rtcpeerconnection via pc on which you simply run the same mediastream commands as you would on simple-peer as they themselves simply run this._pc.addTrack(track, stream) for this.addTrack which has mostly the same functionality
I'm not certain about the re-negotiation stuff

library that has similar features but less bloat

peerjs... but it's in no way lighter

if you ended up using the Stream-like APIs

nope, sending any data runs Buffer.from, so pretty much anything except mediastreams invokes this penalty

would be best addressed by a PR to simple-peer

way ahead of you, I think I have ~12 outstanding PR's to drop readable-stream and buffer all across the webtorrent ecosystem, but they are all stalled as the org is effectively dead, and noone maintains it, so I've been waiting for someone to review them, but that won't happen anytime soon

@ThaUnknown
Copy link
Contributor Author

coincidentally readable-stream is now 50% of the bundle size ;)

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Ok you’ve convinced me it might be worth taking the useful bits of simple-peer and bringing them into a bespoke Peer class - will investigate this when I can. Agree these stream APIs are not very useful and are adding a lot of ugliness to the whole thing. I wasn’t aware of these aspects so thanks for bringing them up.

Also, what about a fork of simple-peer that lands your PRs? Would that also solve this in a way that might have wider impact?

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Fyi here is the pre-simple peer migration. I did this before getting media streams and renegotiation working since I felt after I got the data channel set up it would be ideal to get on the glide path of an existing library to future proof this from webrtc quirks and make it more accessible to people using a known good library.

70f7daf

@ThaUnknown
Copy link
Contributor Author

Ok you’ve convinced me it might be worth taking the useful bits of simple-peer and bringing them into a bespoke Peer class - will investigate this when I can. Agree these stream APIs are not very useful and are adding a lot of ugliness to the whole thing. I wasn’t aware of these aspects so thanks for bringing them up.

Also, what about a fork of simple-peer that lands your PRs? Would that also solve this in a way that might have wider impact?

simple-peer is tightly integrated with stream which means when I went to drop stream I ran into a LOT of issues, and only managed to pass ~90% of the tests, that 10% being enough for the entire lib to fail, so the PR is just sitting there waiting for someone to fix it, or at least read the discord message about it

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Basically once the data channel is set up the whole thing looks like a normal webrtc app with a traditional signalling server (but the renegotiations go over the data channel.)

@ThaUnknown
Copy link
Contributor Author

Basically once the data channel is set up the whole thing looks like a normal webrtc app with a traditional signalling server (but the renegotiations go over the data channel.)

yeah, that's pretty much what polite-peer or perfect-peer does [idk what to call it anymore]

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Yep clean renegotiation is gonna be necessary for whatever we go with - simple-peer does this by asking the initiator to start an offer.

@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

For P2PCF fortunately both peers have a pre-defined role so a similar mechanism can be used. The perfect negotiation pattern of polite-peer is a generalized solution where both peers have no knowledge of one another or coordination beforehand. For P2PCF this isn’t the case since the signaling server helps coordinate the role assignment. (This is part of the protocol because you actually need to assign roles based on the NAT status of both peers to best avoid unnecessary double hole punching and TURN candidates.)

@gfodor gfodor closed this as completed Jul 30, 2022
@gfodor gfodor reopened this Jul 30, 2022
@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

(Sorry accidental button tap :))

@ThaUnknown
Copy link
Contributor Author

I'll pretend like I understood what you said, nod twice and hope you manage to pull some black magic and drop simple-peer sometime soon, I can't wait to be able to use this in my electron projects which can't do signaling via the web push api, which is the hack I used up until now to get signaling for free :p

@gfodor
Copy link
Owner

gfodor commented Jul 31, 2022

#6

@ThaUnknown
Copy link
Contributor Author

🥳 @gfodor with these changes pako, stream we went from 156.9kB/46.9kB to 39.6kB/12.5kB

@gfodor
Copy link
Owner

gfodor commented Jul 31, 2022

Yep way better!

@Yuu6883
Copy link

Yuu6883 commented Aug 1, 2022

Nice package trimming and probably good optimization changes! If you want to even smaller package size you can also try esbuild option --mangle-props=^_ and possibly have a separate production build of tiny-simple-peer that's independent of debug package. That would probably trim off another 10kb uncompressed, even though it's already good enough rn 👏

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

No branches or pull requests

3 participants