Skip to content

use Bytes instead of Vec<u8> for protobuf binary fields#11342

Merged
tdyas merged 3 commits into
pantsbuild:masterfrom
tdyas:prost_use_bytes
Dec 19, 2020
Merged

use Bytes instead of Vec<u8> for protobuf binary fields#11342
tdyas merged 3 commits into
pantsbuild:masterfrom
tdyas:prost_use_bytes

Conversation

@tdyas
Copy link
Copy Markdown
Contributor

@tdyas tdyas commented Dec 18, 2020

Problem

#11307 upgraded the gRPC code to Tonic from grpcio. As part of that upgrade, Pants now uses Prost to generate Rust structs for protobuf types. By default, Prost encodes binary fields as Vec<u8>. Given the attendant problems of Vec<u8> of needing to copy around bytes when structs are cloned, the more efficient method is to use Bytes for the representation to avoid unnecessary copies.

Solution

Upgrade Prost to danburkert/prost@a1cccbc and enable using Bytes for all binary fields.

Result

Existing tests pass.

@tdyas tdyas changed the title use version of Prost with Bytes support use Bytes instead of Vec<u8> for protobuf binary fields Dec 18, 2020
Copy link
Copy Markdown
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread src/rust/engine/Cargo.toml
Copy link
Copy Markdown
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Hurrah!

@tdyas tdyas merged commit 862ed2e into pantsbuild:master Dec 19, 2020
@tdyas tdyas deleted the prost_use_bytes branch December 19, 2020 03:36
tdyas pushed a commit that referenced this pull request Dec 19, 2020
### Problem

Landed #11342 without updating the `prost` crate version in the `concrete_time` crate.

### Solution

Update the revision to conform.

### Result

Existing tests pass.
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.

3 participants