From 3e5113d2d3ecdaec2d759a25a852175a33e6c4d8 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Thu, 3 Oct 2024 11:06:51 -0700 Subject: [PATCH 1/4] Add message and field name to binary serialization errors --- packages/protobuf/src/to-binary.ts | 136 ++++++++++++++++++----------- 1 file changed, 84 insertions(+), 52 deletions(-) diff --git a/packages/protobuf/src/to-binary.ts b/packages/protobuf/src/to-binary.ts index 786e74944..01b8a5564 100644 --- a/packages/protobuf/src/to-binary.ts +++ b/packages/protobuf/src/to-binary.ts @@ -100,20 +100,22 @@ export function writeField( case "enum": writeScalar( writer, + msg.desc.typeName, + field.name, field.scalar ?? ScalarType.INT32, field.number, msg.get(field), ); break; case "list": - writeListField(writer, opts, field, msg.get(field)); + writeListField(writer, msg.desc.typeName, opts, field, msg.get(field)); break; case "message": writeMessageField(writer, opts, field, msg.get(field)); break; case "map": for (const [key, val] of msg.get(field)) { - writeMapEntry(writer, opts, field, key, val); + writeMapEntry(writer, msg.desc.typeName, opts, field, key, val); } break; } @@ -121,12 +123,16 @@ export function writeField( function writeScalar( writer: BinaryWriter, + msgName: string, + fieldName: string, scalarType: ScalarType, fieldNo: number, value: unknown, ) { writeScalarValue( writer.tag(fieldNo, writeTypeOfScalar(scalarType)), + msgName, + fieldName, scalarType, value as ScalarValue, ); @@ -156,6 +162,7 @@ function writeMessageField( function writeListField( writer: BinaryWriter, + msgName: string, opts: BinaryWriteOptions, field: DescField & { fieldKind: "list" }, list: ReflectList, @@ -173,18 +180,25 @@ function writeListField( } writer.tag(field.number, WireType.LengthDelimited).fork(); for (const item of list) { - writeScalarValue(writer, scalarType, item as ScalarValue); + writeScalarValue( + writer, + msgName, + field.name, + scalarType, + item as ScalarValue, + ); } writer.join(); return; } for (const item of list) { - writeScalar(writer, scalarType, field.number, item); + writeScalar(writer, msgName, field.name, scalarType, field.number, item); } } function writeMapEntry( writer: BinaryWriter, + msgName: string, opts: BinaryWriteOptions, field: DescField & { fieldKind: "map" }, key: unknown, @@ -193,13 +207,20 @@ function writeMapEntry( writer.tag(field.number, WireType.LengthDelimited).fork(); // write key, expecting key field number = 1 - writeScalar(writer, field.mapKey, 1, key); + writeScalar(writer, msgName, field.name, field.mapKey, 1, key); // write value, expecting value field number = 2 switch (field.mapKind) { case "scalar": case "enum": - writeScalar(writer, field.scalar ?? ScalarType.INT32, 2, value); + writeScalar( + writer, + msgName, + field.name, + field.scalar ?? ScalarType.INT32, + 2, + value, + ); break; case "message": writeFields( @@ -214,55 +235,66 @@ function writeMapEntry( function writeScalarValue( writer: BinaryWriter, + msgName: string, + fieldName: string, type: ScalarType, value: ScalarValue, ) { - switch (type) { - case ScalarType.STRING: - writer.string(value as string); - break; - case ScalarType.BOOL: - writer.bool(value as boolean); - break; - case ScalarType.DOUBLE: - writer.double(value as number); - break; - case ScalarType.FLOAT: - writer.float(value as number); - break; - case ScalarType.INT32: - writer.int32(value as number); - break; - case ScalarType.INT64: - writer.int64(value as number); - break; - case ScalarType.UINT64: - writer.uint64(value as number); - break; - case ScalarType.FIXED64: - writer.fixed64(value as number); - break; - case ScalarType.BYTES: - writer.bytes(value as Uint8Array); - break; - case ScalarType.FIXED32: - writer.fixed32(value as number); - break; - case ScalarType.SFIXED32: - writer.sfixed32(value as number); - break; - case ScalarType.SFIXED64: - writer.sfixed64(value as number); - break; - case ScalarType.SINT64: - writer.sint64(value as number); - break; - case ScalarType.UINT32: - writer.uint32(value as number); - break; - case ScalarType.SINT32: - writer.sint32(value as number); - break; + try { + switch (type) { + case ScalarType.STRING: + writer.string(value as string); + break; + case ScalarType.BOOL: + writer.bool(value as boolean); + break; + case ScalarType.DOUBLE: + writer.double(value as number); + break; + case ScalarType.FLOAT: + writer.float(value as number); + break; + case ScalarType.INT32: + writer.int32(value as number); + break; + case ScalarType.INT64: + writer.int64(value as number); + break; + case ScalarType.UINT64: + writer.uint64(value as number); + break; + case ScalarType.FIXED64: + writer.fixed64(value as number); + break; + case ScalarType.BYTES: + writer.bytes(value as Uint8Array); + break; + case ScalarType.FIXED32: + writer.fixed32(value as number); + break; + case ScalarType.SFIXED32: + writer.sfixed32(value as number); + break; + case ScalarType.SFIXED64: + writer.sfixed64(value as number); + break; + case ScalarType.SINT64: + writer.sint64(value as number); + break; + case ScalarType.UINT32: + writer.uint32(value as number); + break; + case ScalarType.SINT32: + writer.sint32(value as number); + break; + } + } catch (e) { + if (e instanceof Error) { + throw new Error( + `cannot encode field ${msgName}.${fieldName} to binary: ${e.message}`, + ); + } + throw e; } } From ec56f6f1b6b843c5eebd7db2d8225b816962b0ab Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Mon, 7 Oct 2024 09:42:24 -0700 Subject: [PATCH 2/4] Manually generate bundle size --- packages/bundle-size/README.md | 10 +++++----- packages/bundle-size/chart.svg | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/bundle-size/README.md b/packages/bundle-size/README.md index 9308e7337..cf137ae86 100644 --- a/packages/bundle-size/README.md +++ b/packages/bundle-size/README.md @@ -16,11 +16,11 @@ usually do. We repeat this for an increasing number of files. | code generator | files | bundle size | minified | compressed | | ------------------- | ----: | ----------: | --------: | ---------: | -| Protobuf-ES | 1 | 126,123 b | 65,653 b | 15,258 b | -| Protobuf-ES | 4 | 128,312 b | 67,161 b | 15,970 b | -| Protobuf-ES | 8 | 131,074 b | 68,932 b | 16,481 b | -| Protobuf-ES | 16 | 141,524 b | 76,913 b | 18,797 b | -| Protobuf-ES | 32 | 169,315 b | 98,931 b | 24,268 b | +| Protobuf-ES | 1 | 126,619 b | 65,868 b | 15,324 b | +| Protobuf-ES | 4 | 128,808 b | 67,376 b | 16,007 b | +| Protobuf-ES | 8 | 131,570 b | 69,147 b | 16,553 b | +| Protobuf-ES | 16 | 142,020 b | 77,128 b | 18,849 b | +| Protobuf-ES | 32 | 169,811 b | 99,146 b | 24,363 b | | protobuf-javascript | 1 | 334,193 b | 255,820 b | 42,481 b | | protobuf-javascript | 4 | 360,861 b | 271,092 b | 43,912 b | | protobuf-javascript | 8 | 382,904 b | 283,409 b | 45,038 b | diff --git a/packages/bundle-size/chart.svg b/packages/bundle-size/chart.svg index 2257052d3..618de2e9c 100644 --- a/packages/bundle-size/chart.svg +++ b/packages/bundle-size/chart.svg @@ -43,14 +43,14 @@ 0 KiB - + Protobuf-ES -Protobuf-ES 14.9 KiB for 1 files -Protobuf-ES 15.6 KiB for 4 files -Protobuf-ES 16.09 KiB for 8 files -Protobuf-ES 18.36 KiB for 16 files -Protobuf-ES 23.7 KiB for 32 files +Protobuf-ES 14.96 KiB for 1 files +Protobuf-ES 15.63 KiB for 4 files +Protobuf-ES 16.17 KiB for 8 files +Protobuf-ES 18.41 KiB for 16 files +Protobuf-ES 23.79 KiB for 32 files From 5014953e178022ae9834a5c35cbbf967930ff8d3 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Mon, 7 Oct 2024 10:04:02 -0700 Subject: [PATCH 3/4] Add a test --- packages/protobuf-test/src/binary.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/protobuf-test/src/binary.test.ts b/packages/protobuf-test/src/binary.test.ts index 1a8c23c0b..360faeefe 100644 --- a/packages/protobuf-test/src/binary.test.ts +++ b/packages/protobuf-test/src/binary.test.ts @@ -182,6 +182,16 @@ describe(`binary serialization`, () => { ).toEqual("foo"); }); }); + test("error for invalid data", () => { + const msg = create(ScalarValuesMessageSchema, { + uint32Field: -1, // -1 is invalid for a uint + }); + expect(() => toBinary(ScalarValuesMessageSchema, msg)).toThrow( + new Error( + "cannot encode field spec.ScalarValuesMessage.uint32_field to binary: invalid uint32: -1", + ), + ); + }); }); function testBinary( From 624cc52538dd8656271de8d1569d050b34436b22 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Mon, 7 Oct 2024 11:14:54 -0700 Subject: [PATCH 4/4] Expand perf tests --- packages/protobuf-test/src/perf.ts | 321 ++++++++++++++--------------- 1 file changed, 159 insertions(+), 162 deletions(-) diff --git a/packages/protobuf-test/src/perf.ts b/packages/protobuf-test/src/perf.ts index 9d895c930..55e9cb2fc 100644 --- a/packages/protobuf-test/src/perf.ts +++ b/packages/protobuf-test/src/perf.ts @@ -13,7 +13,14 @@ // limitations under the License. import Benchmark from "benchmark"; -import { create, fromBinary, toBinary, protoInt64 } from "@bufbuild/protobuf"; +import { + create, + fromBinary, + toBinary, + protoInt64, + toJsonString, + fromJsonString, +} from "@bufbuild/protobuf"; import { readFileSync } from "fs"; import { UserSchema } from "./gen/ts/extra/example_pb.js"; import { @@ -95,173 +102,163 @@ interface Test { } function setupTests(): Test[] { - const tests: Test[] = []; - { - const bytes = readFileSync( - new URL("perf-payload.bin", import.meta.url).pathname, - ); - tests.push({ - name: `fromBinary perf-payload.bin`, - fn: () => { - fromBinary(PerfMessageSchema, bytes); - }, - }); - } - { - const desc = UserSchema; - const tinyUser = create(desc, { - active: false, - manager: { active: true }, - }); - const data = toBinary(desc, tinyUser); - tests.push({ - name: `fromBinary tiny example.User (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); - }, - }); - } - { - const desc = UserSchema; - const normalUser = create(desc, { - firstName: "Jane", - lastName: "Doe", - active: true, - manager: { firstName: "Jane", lastName: "Doe", active: false }, - locations: ["Seattle", "New York", "Tokyo"], - projects: { foo: "project foo", bar: "project bar" }, - }); - const data = toBinary(desc, normalUser); - tests.push({ - name: `fromBinary normal example.User (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); - }, - }); - } - { - const desc = ScalarValuesMessageSchema; - const message = create(ScalarValuesMessageSchema, { - doubleField: 0.75, - floatField: -0.75, - int64Field: protoInt64.parse(-1), - uint64Field: protoInt64.uParse(1), - int32Field: -123, - fixed64Field: protoInt64.uParse(1), - fixed32Field: 123, - boolField: true, - stringField: "hello world", - bytesField: new Uint8Array([ - 104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, - ]), - uint32Field: 123, - sfixed32Field: -123, - sfixed64Field: protoInt64.parse(-1), - sint32Field: -1, - sint64Field: protoInt64.parse(-1), - }); - const data = toBinary(desc, message); - tests.push({ - name: `fromBinary scalar values (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); - }, - }); - } - { - const desc = RepeatedScalarValuesMessageSchema; - const message = create(desc, { - doubleField: [0.75, 0, 1], - floatField: [0.75, -0.75], - int64Field: [protoInt64.parse(-1), protoInt64.parse(-2)], - uint64Field: [protoInt64.uParse(1), protoInt64.uParse(2)], - int32Field: [-123, 500], - fixed64Field: [protoInt64.uParse(1), protoInt64.uParse(99)], - fixed32Field: [123, 999], - boolField: [true, false, true], - stringField: ["hello", "world"], - bytesField: [ - new Uint8Array([104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100]), - ], - uint32Field: [123, 123], - sfixed32Field: [-123, -123, -123], - sfixed64Field: [ - protoInt64.parse(-1), - protoInt64.parse(-2), - protoInt64.parse(100), - ], - sint32Field: [-1, -2, 999], - sint64Field: [ - protoInt64.parse(-1), - protoInt64.parse(-99), - protoInt64.parse(99), - ], - }); - const data = toBinary(desc, message); - tests.push({ - name: `fromBinary repeated scalar fields (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); - }, - }); - } - { - const desc = MapsMessageSchema; - const message = create(desc, { - strStrField: { a: "str", b: "xx" }, - strInt32Field: { a: 123, b: 455 }, - strInt64Field: { a: protoInt64.parse(123) }, - strBoolField: { a: true, b: false }, - strBytesField: { - a: new Uint8Array([ + const tests: Test[] = [ + { + name: "perf-payload.bin", + desc: PerfMessageSchema, + msg: fromBinary( + PerfMessageSchema, + readFileSync(new URL("perf-payload.bin", import.meta.url).pathname), + ), + }, + { + name: "tiny example.User", + desc: UserSchema, + msg: create(UserSchema, { active: false, manager: { active: true } }), + }, + { + name: "normal example.User", + desc: UserSchema, + msg: create(UserSchema, { + firstName: "Jane", + lastName: "Doe", + active: true, + manager: { firstName: "Jane", lastName: "Doe", active: false }, + locations: ["Seattle", "New York", "Tokyo"], + projects: { foo: "project foo", bar: "project bar" }, + }), + }, + { + name: "scalar values", + desc: ScalarValuesMessageSchema, + msg: create(ScalarValuesMessageSchema, { + doubleField: 0.75, + floatField: -0.75, + int64Field: protoInt64.parse(-1), + uint64Field: protoInt64.uParse(1), + int32Field: -123, + fixed64Field: protoInt64.uParse(1), + fixed32Field: 123, + boolField: true, + stringField: "hello world", + bytesField: new Uint8Array([ 104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, ]), + uint32Field: 123, + sfixed32Field: -123, + sfixed64Field: protoInt64.parse(-1), + sint32Field: -1, + sint64Field: protoInt64.parse(-1), + }), + }, + { + name: "repeated scalar values", + desc: RepeatedScalarValuesMessageSchema, + msg: create(RepeatedScalarValuesMessageSchema, { + doubleField: [0.75, 0, 1], + floatField: [0.75, -0.75], + int64Field: [protoInt64.parse(-1), protoInt64.parse(-2)], + uint64Field: [protoInt64.uParse(1), protoInt64.uParse(2)], + int32Field: [-123, 500], + fixed64Field: [protoInt64.uParse(1), protoInt64.uParse(99)], + fixed32Field: [123, 999], + boolField: [true, false, true], + stringField: ["hello", "world"], + bytesField: [ + new Uint8Array([ + 104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, + ]), + ], + uint32Field: [123, 123], + sfixed32Field: [-123, -123, -123], + sfixed64Field: [ + protoInt64.parse(-1), + protoInt64.parse(-2), + protoInt64.parse(100), + ], + sint32Field: [-1, -2, 999], + sint64Field: [ + protoInt64.parse(-1), + protoInt64.parse(-99), + protoInt64.parse(99), + ], + }), + }, + { + name: "map with scalar keys and values", + desc: MapsMessageSchema, + msg: create(MapsMessageSchema, { + strStrField: { a: "str", b: "xx" }, + strInt32Field: { a: 123, b: 455 }, + strInt64Field: { a: protoInt64.parse(123) }, + strBoolField: { a: true, b: false }, + strBytesField: { + a: new Uint8Array([ + 104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, + ]), + }, + int32StrField: { 123: "hello" }, + int64StrField: { "9223372036854775807": "hello" }, + boolStrField: { true: "yes", false: "no" }, + strEnuField: { a: 0, b: 1, c: 2 }, + int32EnuField: { 1: 0, 2: 1, 0: 2 }, + int64EnuField: { "-1": 0, "2": 1, "0": 2 }, + }), + }, + { + name: "repeated field with 1000 messages", + desc: MessageFieldMessageSchema, + msg: (() => { + const message = create(MessageFieldMessageSchema); + for (let i = 0; i < 1000; i++) { + message.repeatedMessageField.push( + create(MessageFieldMessage_TestMessageSchema), + ); + } + return message; + })(), + }, + { + name: "map field with 1000 messages", + desc: MapsMessageSchema, + msg: (() => { + const message = create(MapsMessageSchema); + for (let i = 0; i < 1000; i++) { + message.strMsgField[i.toString()] = create(MapsMessageSchema); + } + return message; + })(), + }, + ].flatMap(({ name, msg, desc }) => { + const bytes = toBinary(desc, msg); + const jsonString = toJsonString(desc, msg); + return [ + { + name: `fromBinary ${name}`, + fn: () => { + fromBinary(desc, bytes); + }, }, - int32StrField: { 123: "hello" }, - int64StrField: { "9223372036854775807": "hello" }, - boolStrField: { true: "yes", false: "no" }, - strEnuField: { a: 0, b: 1, c: 2 }, - int32EnuField: { 1: 0, 2: 1, 0: 2 }, - int64EnuField: { "-1": 0, "2": 1, "0": 2 }, - }); - const data = toBinary(desc, message); - tests.push({ - name: `fromBinary map with scalar keys and values (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); + { + name: `fromJson ${name}`, + fn: () => { + fromJsonString(desc, jsonString); + }, }, - }); - } - { - const desc = MessageFieldMessageSchema; - const message = create(desc); - for (let i = 0; i < 1000; i++) { - message.repeatedMessageField.push( - create(MessageFieldMessage_TestMessageSchema), - ); - } - const data = toBinary(desc, message); - tests.push({ - name: `fromBinary repeated field with 1000 messages (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); + { + name: `toBinary ${name}`, + fn: () => { + toBinary(desc, msg); + }, }, - }); - } - { - const desc = MapsMessageSchema; - const message = create(desc); - for (let i = 0; i < 1000; i++) { - message.strMsgField[i.toString()] = create(desc); - } - const data = toBinary(desc, message); - tests.push({ - name: `fromBinary map field with 1000 messages (${data.byteLength} bytes)`, - fn: () => { - fromBinary(desc, data); + { + name: `toJson ${name}`, + fn: () => { + toJsonString(desc, msg); + }, }, - }); - } + ]; + }); return tests; }