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

Binary WebSocket API: Brotli-compress all outgoing messages #1026

Merged
merged 4 commits into from
Mar 29, 2024

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Mar 26, 2024

Description of Changes

Simpler solution to #1022 , compared to #1021 or #1023 .

With this commit, all outgoing messages using the binary protocol (i.e. Protobuf) are Brotli-compressed at level 1 (fastest, least compression).

API and ABI breaking changes

Unsure which breakage label is correct. Breaks the binary WebSocket API, and so will require changes to:

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

3 - breaking the WebSocket API is scary.

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Rust SDK tests pass.
  • After updating Typescript SDK, run the quickstart-chat app.
  • After updating C# SDK, run the quickstart-chat app (thanks @RReverser !).
  • After updating Unity SDK, run BitCraft.
    • Run with bots and ensure server performance doesn't regress due to added compression work.
    • Verify that latency on resubscribing (i.e. crossing a chunk boundary) is reduced or eliminated.

@gefjon gefjon added bitcraft issue Active issue for the BitCraft team performance breaks library compatibility This change breaks the SpacetimeDB library interface test-with-bots Recommend to test under higher CCU labels Mar 26, 2024
gefjon added a commit to clockworklabs/spacetime-docs that referenced this pull request Mar 26, 2024
Co-authored-by: joshua-spacetime <[email protected]>
Signed-off-by: Phoebe Goldman <[email protected]>
Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment about reducing the buffer size to 32KB, but otherwise is good.

@joshua-spacetime joshua-spacetime linked an issue Mar 26, 2024 that may be closed by this pull request
gefjon added a commit to clockworklabs/spacetimedb-typescript-sdk that referenced this pull request Mar 27, 2024
crates/core/src/client/messages.rs Show resolved Hide resolved
RReverser added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Mar 27, 2024
@bfops
Copy link
Collaborator

bfops commented Mar 28, 2024

Bot test results

Conclusion: 🤷 🧐

Note: eval_binary and enemy_ai_agent_loop seem to have far fewer calls in the "after" version. I don't know why this would be, unless they are called less frequently under load?

call_reducer

image

call_reducer_with_tx

image

eval_incr

image

eval_updates

image

eval_binary

image

enemy_ai_agent_loop

image

npc_ai_agent_loop

image

Commits tested

before:

commit 026e011f40613b3470931c9ed6c4c8632ce4bb94 (HEAD -> bfops/perf-test-before, origin/bfops/perf-test-before)
Merge: 9141a426 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 17:33:08 2024 -0700

    [bfops/perf-test-before]: Merge remote-tracking branch 'origin/bfops/enable-tracy' into bfops/perf-test-before

commit 9141a426229a7a9bd9663fa5b2c435ec28e09404
Author: Mazdak Farrokhzad <[email protected]>
Date:   Mon Mar 25 21:27:08 2024 +0100

    Bump Rust to 1.77 + fix warnings + use `Bound::map` (#1020)
    
    * bump Rust to 1.77 + fix warnings + use Bound::map
    
    * use .truncate(true) for OpenOptions

after:

commit a7436e5ed2ae031f39907d08d10eba8baba267c7 (origin/bfops/perf-test-after)
Merge: 0fdc0050 0ad01b77
Author: Zeke Foppa <[email protected]>
Date:   Wed Mar 27 16:37:37 2024 -0700

    [bfops/perf-test-after]: Merge remote-tracking branch 'origin/bfops/enable-tracy' into bfops/perf-test-after

commit 0fdc005054deadbb53d06184fff6f190ac2537a0 (origin/phoebe/compress-websocket-messages)
Author: Phoebe Goldman <[email protected]>
Date:   Tue Mar 26 15:58:22 2024 -0400

    Decrease buffer size; comment on future work
    
    Co-authored-by: joshua-spacetime <[email protected]>
    Signed-off-by: Phoebe Goldman <[email protected]>

commit c1e322fd1fc6e0985649fc54052808498314faa7
Author: Phoebe Goldman <[email protected]>
Date:   Tue Mar 26 14:16:09 2024 -0400

    Binary WebSocket API: Brotli-compress all outgoing messages

commit 9141a426229a7a9bd9663fa5b2c435ec28e09404
Author: Mazdak Farrokhzad <[email protected]>
Date:   Mon Mar 25 21:27:08 2024 +0100

    Bump Rust to 1.77 + fix warnings + use `Bound::map` (#1020)
    
    * bump Rust to 1.77 + fix warnings + use Bound::map
    
    * use .truncate(true) for OpenOptions

@gefjon
Copy link
Contributor Author

gefjon commented Mar 29, 2024

Results posted in other channels have shown this to significantly improve (though not eliminate) network latency for large SubscriptionUpdate messages. Merging.

gefjon pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Mar 29, 2024
@gefjon gefjon added this pull request to the merge queue Mar 29, 2024
Merged via the queue into master with commit 2d971f3 Mar 29, 2024
6 checks passed
gefjon added a commit to clockworklabs/spacetimedb-typescript-sdk that referenced this pull request Mar 29, 2024
RReverser pushed a commit that referenced this pull request Apr 17, 2024
* Binary WebSocket API: Brotli-compress all outgoing messages

* Decrease buffer size; comment on future work

Co-authored-by: joshua-spacetime <[email protected]>
Signed-off-by: Phoebe Goldman <[email protected]>

* Note experimental compression ratio

---------

Signed-off-by: Phoebe Goldman <[email protected]>
Co-authored-by: joshua-spacetime <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcraft issue Active issue for the BitCraft team breaks library compatibility This change breaks the SpacetimeDB library interface test-with-bots Recommend to test under higher CCU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compress protobuf messages using brotli
4 participants