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

feat(build): Custom codecs for generated code #1599

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

kvcache
Copy link
Contributor

@kvcache kvcache commented Jan 12, 2024

Motivation

My company has a pubsub service powered in part by Tonic. It is designed to accept
hundreds of thousands of subscriptions per EC2 instance. Subscriptions normally
receive infrequent, very small messages - to the tune of 200 bytes. It is rare that a
message is over 1kb, and the service has a cap at 4kb.

I observed that memory is the limiting factor for reaching the required memory
density goals. I've tuned our structs and removed as many extra vtable references
as possible. I even gave a concrete stream type up to tonic in the subscription rpc
implementation to save 8 bytes per subscription. These optimizations, per
Amdahl's Law, can only save on the order of 2% memory (in my system) with
16 kilobytes allocated to encoding and decoding per stream.

I'm not the only person at my company who writes services, and our ecosystem that
touches protocol buffers is multi-language. Generated code helps to ensure
consistency in cross-language service vs client definition. For these reasons and
others, we strongly prefer to keep the tonic service traits as generated code. The
Tonic code generator is awesome!

I experimented with changing decode.rs's const BUFFER_SIZE: usize = 8 * 1024; to
smaller values, and it did the needful. This PR is an attempt to make it configurable.

Solution

Broadly, this change does 2 things:

  1. Allow the built-in Prost codec to have its buffer sizes customized
  2. Allow users to specify custom codecs on the tonic_build::prost::Builder

The Prost codec is convenient, and handles any normal use case. However,
the buffer sizes today are too large in some cases - and they may grow too
aggressively. By exposing BufferSettings, users can make a small custom
codec with their own BufferSettings to control their memory usage - or give
enormous buffers to rpc's, as their use case requires.

While one can define a custom service and methods with a custom codec today
explicitly in Rust, the code generator does not have a means to supply a
custom codec. I've reached for .codec... on the tonic_build::prost::Builder
many times and keep forgetting it's not there. This change adds .codec_path
to the Builder, so people can simply add their custom buffer codec or even
their own full top level codec without reaching for manual service definition.

I've included a hello world example with a customized codec similar to that which
I'm currently compiling in my service.

By exposing ProstEncoder/ProstDecoder, BufferSettings, and a
tonic_build::prost::Builder::codec_path setting, I am able to meet the memory
density requirements for our small item, low throughput streaming rpcs. I am also
able to separate the buffer size settings between small message Services and large
message Services.

kvcache added a commit to kvcache/tonic that referenced this pull request Jan 18, 2024
Broadly, this change does 2 things:
1. Allow the built-in Prost codec to have its buffer sizes customized
2. Allow users to specify custom codecs on the tonic_build::prost::Builder

The Prost codec is convenient, and handles any normal use case. However,
the buffer sizes today are too large in some cases - and they may grow too
aggressively. By exposing BufferSettings, users can make a small custom
codec with their own BufferSettings to control their memory usage - or give
enormous buffers to rpc's, as their use case requires.

While one can define a custom service and methods with a custom codec today
explicitly in Rust, the code generator does not have a means to supply a
custom codec. I've reached for .codec... on the tonic_build::prost::Builder
many times and keep forgetting it's not there. This change adds .codec_path
to the Builder, so people can simply add their custom buffer codec or even
their own full top level codec without reaching for manual service definition.

This change is cherry picked from hyperium#1599
and applied on top of v0.10.2 release to investigate a p999 latency increase.
Broadly, this change does 2 things:
1. Allow the built-in Prost codec to have its buffer sizes customized
2. Allow users to specify custom codecs on the tonic_build::prost::Builder

The Prost codec is convenient, and handles any normal use case. However,
the buffer sizes today are too large in some cases - and they may grow too
aggressively. By exposing BufferSettings, users can make a small custom
codec with their own BufferSettings to control their memory usage - or give
enormous buffers to rpc's, as their use case requires.

While one can define a custom service and methods with a custom codec today
explicitly in Rust, the code generator does not have a means to supply a
custom codec. I've reached for .codec... on the tonic_build::prost::Builder
many times and keep forgetting it's not there. This change adds .codec_path
to the Builder, so people can simply add their custom buffer codec or even
their own full top level codec without reaching for manual service definition.
@kvcache
Copy link
Contributor Author

kvcache commented Feb 8, 2024

Force-pushed commit to rebase on the tip of master. Force push was used because this hasn't been reviewed yet.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall looks really good left some feedback on how we can avoid some breaking changes but I think the idea is following down the right path, thanks!

tonic-build/src/compile_settings.rs Outdated Show resolved Hide resolved
tonic-build/src/lib.rs Outdated Show resolved Hide resolved
tonic-build/src/prost.rs Outdated Show resolved Hide resolved
tonic/src/codec/mod.rs Outdated Show resolved Hide resolved
tonic/src/codec/mod.rs Outdated Show resolved Hide resolved
tonic/src/codec/mod.rs Outdated Show resolved Hide resolved
@kvcache kvcache requested a review from LucioFranco February 14, 2024 00:19
@LucioFranco
Copy link
Member

CI is failing due to

  error: unused import: `compile_settings::CompileSettings`
     --> tonic-build/src/lib.rs:101:16
      |
  101 | pub(crate) use compile_settings::CompileSettings;
      |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: `-D unused-imports` implied by `-D warnings`
      = help: to override `-D warnings` add `#[allow(unused_imports)]`
  
  error: field `codec_path` is never read
   --> tonic-build/src/compile_settings.rs:3:16
    |
  2 | pub(crate) struct CompileSettings {
    |                   --------------- field in this struct
  3 |     pub(crate) codec_path: String,
    |                ^^^^^^^^^^
    |
    = note: `CompileSettings` has derived impls for the traits `Clone` and `Debug`, but these are intentionally ignored during dead code analysis
    = note: `-D dead-code` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(dead_code)]`

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall looks really good, I think there are a few warnings/CI fixes needed and I left a few comments. I think we can merge it once its all fixed.

tonic/src/codec/mod.rs Show resolved Hide resolved
examples/src/json-codec/common.rs Outdated Show resolved Hide resolved
tonic/benches/decode.rs Outdated Show resolved Hide resolved
@kvcache kvcache requested a review from LucioFranco February 15, 2024 22:50
@kvcache
Copy link
Contributor Author

kvcache commented Feb 15, 2024

I installed nightly, cargo-hack, and cargo-udeps to reproduce the workflow error locally. I addressed it by gating the codec path on the prost feature. If it's needed outside of that feature, I think it can be consumed and the feature gates removed.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks!

@LucioFranco LucioFranco changed the title feat(tonic): Custom codecs for generated code feat(build): Custom codecs for generated code Feb 20, 2024
@LucioFranco LucioFranco added this pull request to the merge queue Feb 20, 2024
Merged via the queue into hyperium:master with commit 18a2b30 Feb 20, 2024
16 checks passed
@kvcache kvcache deleted the codecs branch February 20, 2024 21:13
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