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

Switch from bincode to postcard #410

Merged
merged 6 commits into from
Feb 7, 2025
Merged

Switch from bincode to postcard #410

merged 6 commits into from
Feb 7, 2025

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Feb 7, 2025

I recently discovered that deserialization from std::io::Cursor allocates because it needs a temporary buffer to read data into. Here is what bincode uses.

RC version of bincode provides a special Reader additionally to read from it. Unfortunately, I don't think bincode will be released soon, see this comment.

This is why I decided to try postcard. Difference from bincode

  • Variable integers by default and It's encoded the way we want. So we don't need to use integer-encoding crate anymore and all integers inside components get this optimization (can be opt-out via attribute!).
  • I like postcard API more. DefaultOptions::new() from bincode was always seemed off to me. In the latest RC they pass settings to each call which is even more verbose. postcard have a very flexible Flavor system.
  • Potentially can work with no_std. Although, RC version of bincode also supports it.
  • postcard::Error is not an alias for a boxed error, similar to bincode RC.

I compered the perfomance, we didn't get any improvements in our benchmarks, it's within the margin. But I think it's worth to switch.

To get postcard play nicely with streaming ser/de, I added Flavors and functions similar to what postcard provides.

  • For serialization I implemented ExtendFlavorMut, which is similar to the provided ExtendFlavor, but accepts value by reference.
  • For deserialization I implemented BufFlavor which works with any type that implements bytes::Buf. This allows me to read from Bytes directly without extra allocations or manually reassigning the value like with built-in SliceFlavor. I re-exported Bytes from the crate.

I also created a newtype for mutation indexes to mark it as fixint-serializable with postcard.

Since we have more suited variable integers, it might worth to discuss using varints for ticks. Previously, ticks started to use 5 bytes after 2^16 (which happens after ~18 minutes with 60 ticks/s).
But right now it's after 2^28 and with 60 ticks/s it starts to take 5 bytes after ~51 day.
If you agree, I will push it as a follow-up PR. This PR only swaps the API without any logical changes.

@Shatur Shatur requested a review from UkoeHB February 7, 2025 00:43
@Shatur
Copy link
Contributor Author

Shatur commented Feb 7, 2025

Hm... Statistic test says that we consume one more byte 🤔 I will investigate tomorrow, I didn't expect it.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 87.77778% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.47%. Comparing base (ed1d294) to head (84d3dc2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/core/postcard_utils.rs 70.00% 9 Missing ⚠️
src/core/entity_serde.rs 57.14% 3 Missing ⚠️
src/core/event/server_event.rs 81.25% 3 Missing ⚠️
src/core/replication/replicated_clients.rs 50.00% 2 Missing ⚠️
src/server/replication_messages/update_message.rs 87.50% 2 Missing ⚠️
src/client.rs 97.72% 1 Missing ⚠️
...re/replication/replication_registry/command_fns.rs 66.66% 1 Missing ⚠️
src/server/replication_messages/serialized_data.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   89.75%   89.47%   -0.29%     
==========================================
  Files          51       53       +2     
  Lines        2773     2793      +20     
==========================================
+ Hits         2489     2499      +10     
- Misses        284      294      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shatur
Copy link
Contributor Author

Shatur commented Feb 7, 2025

@UkoeHB I looked into the difference. I forgot that we used variable integer encoding for update indices and switched them to fixints in this PR. But I think it's good for the same reason we made ticks use fixints. What do you think?

If you agree, then the PR is ready for review.

Copy link
Collaborator

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

The implicit varint encoding is a bit confusing since you can't infer it from the callsite.

src/server/replication_messages/component_changes.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor Author

Shatur commented Feb 7, 2025

I agree!
But on the bright side when it's associated with a type I don't have to worry about matching serialization and deserialization 🙂

What do you think about the tick? Do you agree about with turning into varint?

@Shatur Shatur merged commit 753e5d8 into master Feb 7, 2025
@Shatur Shatur deleted the postcard branch February 7, 2025 23:00
@UkoeHB
Copy link
Collaborator

UkoeHB commented Feb 7, 2025

What do you think about the tick? Do you agree about with turning into varint?

Do you mean the update indices being fixed int? It is fine yes.

@Shatur
Copy link
Contributor Author

Shatur commented Feb 7, 2025

No, no, I mean the RepliconTick 🙂 I talking about this:

Since we have more suited variable integers, it might worth to discuss using varints for ticks. Previously, ticks started to use 5 bytes after 2^16 (which happens after ~18 minutes with 60 ticks/s).
But right now it's after 2^28 and with 60 ticks/s it starts to take 5 bytes after ~51 day.
If you agree, I will push it as a follow-up PR. This PR only swaps the API without any logical changes.

It's because it uses u32.

@UkoeHB
Copy link
Collaborator

UkoeHB commented Feb 7, 2025

Ah ok, yes we can switch to varints for u32 ticks.

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