search

gfodor / p2pcf

Low cost, low effort P2P WebRTC serverless signalling using Cloudflare Workers
MIT License
964 stars 53 forks source link

fix: drop pako #2

Closed ThaUnknown closed 2 years ago

ThaUnknown commented 2 years ago

This drops pako as a dependency. From my [limited] testing, pako is only used for ~5% of all requests and saving just a few bytes, while making up almost 40% of the bundle size for the module. The size optimization brought in by pako likely won't actually reduce the amt of data transferred over the network as the packet size would most likely be too small to split it anyways.

gfodor commented 2 years ago

Wow thanks for the PR! The reason I added the deflation is that for larger rooms the package collections can bloat to several kb but are very compressible. I’m def willing to be convinced this is a bad idea but maybe there is a way to deal with this without taking on the dependency?

ThaUnknown commented 2 years ago

could we maybe make a reproductible test case for this? I did see some minor increases, but it was at most 200 bytes?

for alternatives maybe we could strip unnecessary data from such requests or something along the lines? but for that we'd need a test case

gfodor commented 2 years ago

I’m afk for a while, but try popping 5-6 tabs of the demo and adding a logging breakpoint in the debugger that writes the length of the naive JSON stringified payload. Iirc it’s somewhere between 5:1 and 10:1 once there are one or two packages in flight.

gfodor commented 2 years ago

(Afk meaning I can’t write a test for this until later - I am not offline - but this is a good excuse to add a test suite :))

ThaUnknown commented 2 years ago

I ca reproduce this with >4 peers in a room, where deflate manages to half the data image it definitely scales with the amt of users, but I'm not sure what this handshake does [it only happens on initial connection] the rest of the requests are fine, there seems to be a lot of duplicate data bout peers we're already connected to

I don't really know the nitty gritty of how you've implemented this, but is it necessary? image

ThaUnknown commented 2 years ago

but is it necessary?

this is heading in a bad direction, as it talks about implementation details, because if you decide to migrate to polite-peer, then it might behave marginally differently, but this seems more like lobby controls rather than signaling so that might not be true

gfodor commented 2 years ago

Yep those are the signalling candidates and other data needed to negotiate with a peer. The reason I send them every request is it makes correctness easier and reduces the complexity of the worker, but in theory the worker could do more bookkeeping and ACK back the receipt of the packages you have posted. The edge cases felt gnarly so I punted on them - the payload size drops after a few minutes so I threw deflate at the problem but agree the dependency bloats it significantly.

gfodor commented 2 years ago

Oh you know what, those used to be posted every request when I had the KV implementation which didn’t have read-after-write semantics - so it was strictly necessary. I forgot now I sent them only when they change so perhaps we can drop the dependency. Can you confirm that those big payloads aren’t being sent on every poll now? This might be a residual decision that should have been unwound once I dropped support for KV. (See the response to the question in issues.

ThaUnknown commented 2 years ago

Oh you know what, those used to be posted every request when I had the KV implementation which didn’t have read-after-write semantics - so it was strictly necessary. I forgot now I sent them only when they change so perhaps we can drop the dependency. Can you confirm that those big payloads aren’t being sent on every poll now? This might be a residual decision that should have been unwound once I dropped support for KV. (See the response to the question in issues.

if by poll you mean the update that runs on an interval, then yes, this ONLY runs when a new peer connects, and the following interval keepalives are smaller in raw json than deflate

gfodor commented 2 years ago

Ok that makes sense. Yeah I think the trade off flips the other direction then and we can drop pako. I can probably write a more efficient JSON encoding at some point.

ThaUnknown commented 2 years ago

Ok that makes sense. Yeah I think the trade off flips the other direction then and we can drop pako. I can probably write a more efficient JSON encoding at some point.

okay, fixing conflict

gfodor commented 2 years ago

Thank you!

ThaUnknown commented 2 years ago

image