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

fix: drop pako #2

Merged
merged 2 commits into from
Jul 30, 2022
Merged

fix: drop pako #2

merged 2 commits into from
Jul 30, 2022

Conversation

ThaUnknown
Copy link
Contributor

@ThaUnknown ThaUnknown commented Jul 30, 2022

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
Copy link
Owner

gfodor commented Jul 30, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

gfodor commented Jul 30, 2022

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
Copy link
Owner

gfodor commented Jul 30, 2022

(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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

gfodor commented Jul 30, 2022

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
Copy link
Owner

gfodor commented Jul 30, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

gfodor commented Jul 30, 2022

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
Copy link
Contributor Author

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 gfodor merged commit 45d1ba7 into gfodor:master Jul 30, 2022
@gfodor
Copy link
Owner

gfodor commented Jul 30, 2022

Thank you!

@ThaUnknown
Copy link
Contributor Author

image

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.

2 participants