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

Add message and field name to binary serialization errors #984

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bhollis
Copy link
Contributor

@bhollis bhollis commented Oct 3, 2024

This adds the message and field name to errors thrown during binary serialization of scalar fields. It should convert a message like:

Error: invalid uint32: -1

To:

Error: cannot encode field com.whatever.MyMessage.my_field to binary: invalid uint32: -1

I followed the wording of the error thrown here for the phrasing: https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/src/to-binary.ts#L74

Fixes #983

@timostamm
Copy link
Member

Thanks for the PR, Ben. We'll need some insight whether / how this impacts performance and bundle size. I think it would be ideal to use the same mechanism for error messages that toJson uses (but writeField is also used for extensions). We need good test coverage to make sure error messages have the expected content and will not regress.

@bhollis
Copy link
Contributor Author

bhollis commented Oct 7, 2024

Running npm run perf in packages/protobuf-test:

Edit: Hmm, looks like there aren't any performance tests for toBinary. Let me add them.

Before:

toBinary   perf-payload.bin x 5,528 ops/sec ±4.33% (94 runs sampled)
toBinary   tiny example.User x 1,098,031 ops/sec ±0.35% (95 runs sampled)
toBinary   normal example.User x 177,436 ops/sec ±1.62% (95 runs sampled)
toBinary   scalar values x 275,100 ops/sec ±0.76% (97 runs sampled)
toBinary   repeated scalar values x 94,595 ops/sec ±0.64% (98 runs sampled)
toBinary   map with scalar keys and values x 65,719 ops/sec ±0.58% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,984 ops/sec ±0.56% (98 runs sampled)
toBinary   map field with 1000 messages x 729 ops/sec ±0.71% (97 runs sampled)

After:

toBinary   perf-payload.bin x 5,452 ops/sec ±1.57% (97 runs sampled)
toBinary   tiny example.User x 1,076,997 ops/sec ±0.32% (96 runs sampled)
toBinary   normal example.User x 182,993 ops/sec ±0.56% (98 runs sampled)
toBinary   scalar values x 277,949 ops/sec ±0.55% (95 runs sampled)
toBinary   repeated scalar values x 90,580 ops/sec ±3.47% (92 runs sampled)
toBinary   map with scalar keys and values x 63,867 ops/sec ±0.62% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,956 ops/sec ±0.51% (97 runs sampled)
toBinary   map field with 1000 messages x 696 ops/sec ±0.79% (94 runs sampled)

@@ -95,173 +102,163 @@ interface Test {
}

function setupTests(): Test[] {
const tests: Test[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a huge change but I'm just switching to generating the benchmark cases instead of having to repeat code.

Comment on lines +235 to +237
return [
{
name: `fromBinary ${name}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual change - we now benchmark fromBinary, fromJsonString, toBinary, and toJsonString for each example message.

@timostamm
Copy link
Member

Thanks for the updates, Ben. I'll take a look as soon as possible, but have to take care of some Connect work first.

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.

Include field and type name in toBinary errors?
2 participants