Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(codec): revert to tokio-util traits #773

Closed
wants to merge 1 commit into from

Conversation

themaxdavitt
Copy link

Motivation

See the discussion post I made a couple of weeks ago.

Pull request #208 introduced tonic::codec::{DecodeBuf, Decoder, EncodeBuf, Encoder} in preparation for experimenting with a different decoding strategy, but as it currently stands the in-tree functionality is equivalent to the original implementation using tokio_util::codec::{Decoder, Encoder} and bytes::BytesMut.

Solution

Given that a significant amount of time has passed with no experimentation having happened (as far as I can tell), this commit reverts those changes. Additionally, it also restores efficient use of the underlying buffers, an issue that was brought up later in its respective comment thread.

Benchmarks

The performance impact is negligible, but still worth noting. I ran this on my 2020 13-inch M1 MacBook Pro (16GB RAM), if that matters.

Before:

test chunk_size_100   ... bench:         566 ns/iter (+/- 8) = 1775 MB/s
test chunk_size_1005  ... bench:         361 ns/iter (+/- 13) = 2783 MB/s
test chunk_size_500   ... bench:         412 ns/iter (+/- 13) = 2439 MB/s
test message_count_1  ... bench:         344 ns/iter (+/- 9) = 1468 MB/s
test message_count_10 ... bench:       1,344 ns/iter (+/- 71) = 3757 MB/s
test message_count_20 ... bench:       2,468 ns/iter (+/- 84) = 4092 MB/s
test message_size_10k ... bench:       1,654 ns/iter (+/- 151) = 12097 MB/s
test message_size_1k  ... bench:         495 ns/iter (+/- 30) = 4060 MB/s
test message_size_5k  ... bench:         922 ns/iter (+/- 40) = 10856 MB/s

After:

test chunk_size_100   ... bench:         600 ns/iter (+/- 17) = 1675 MB/s
test chunk_size_1005  ... bench:         394 ns/iter (+/- 14) = 2550 MB/s
test chunk_size_500   ... bench:         442 ns/iter (+/- 18) = 2273 MB/s
test message_count_1  ... bench:         387 ns/iter (+/- 16) = 1304 MB/s
test message_count_10 ... bench:         993 ns/iter (+/- 53) = 5085 MB/s
test message_count_20 ... bench:       1,669 ns/iter (+/- 25) = 6051 MB/s
test message_size_10k ... bench:       1,284 ns/iter (+/- 106) = 15584 MB/s
test message_size_1k  ... bench:         469 ns/iter (+/- 18) = 4285 MB/s
test message_size_5k  ... bench:         788 ns/iter (+/- 39) = 12703 MB/s

(I ran cargo bench 2>&1 | grep bench: to produce these figures, my apologies if I am missing anything)

Pull request hyperium#208 introduced `tonic::codec::{DecodeBuf, Decoder, EncodeBuf, Encoder}`
in preparation for experimenting with a different decoding strategy, but as it
currently stands the in-tree functionality is equivalent to the original
implementation using `tokio_util::codec::{Decoder, Encoder}` and
`bytes::BytesMut`. Given that a significant amount of time has passed with no
experimentation having happened (as far as I can tell), this commit reverts
those changes. Additionally, it also restores efficient use of the underlying
buffers, an issue that was brought up later in its respective comment thread.
@LucioFranco
Copy link
Member

We will be keeping the tonic codec traits for versioning purposes. Thanks for the contribution!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants