features/unmarshal: implement unmarshal_unique #140
Conversation
Implement unmarshal_unique that is like unmarshal but calls unique.Make() on each string. The idea is to keep only one copy of a string around with gRPC marshaling/unmarshaling pools on. No idea how to test this in unit tests but this change works in our project. Signed-off-by: Giedrius Statkevičius <[email protected]>
Convert the option into a field option. Update README.md. Signed-off-by: Giedrius Statkevičius <[email protected]>
|
@vmg thanks for the very quick review 🙇 I have added |
There was a problem hiding this comment.
Love the new implementation. Could you please add some tests for the new feature? Main thing to test would be that unmarshalling the same message twice results in the same string. You can use https://pkg.go.dev/unsafe#StringData to verify the the string has been properly interned. Thank you!
Signed-off-by: Giedrius Statkevičius <[email protected]>
|
👍 🚀 added a test using |
| } | ||
|
|
||
| extend google.protobuf.FieldOptions { | ||
| optional Opts options = 999999; |
There was a problem hiding this comment.
What happened here? Why is this set to 999999? This seems like something that could conflict. Did the previous 64102 collide or such?
There was a problem hiding this comment.
It's my first time writing a field option. How do we know if any number doesn't collide with some other number that someone chose?
There was a problem hiding this comment.
There's no way to be 100%, but the idea is namespacing the field options with a high enough number. In this case we've choseen 641xx as the ID range for vtprotobuf, so any number in that range would be reasonable to use. 999999 is, well, a number unlikely to collide either, but it's outside of the range we've chosen for the library.
There was a problem hiding this comment.
A good number to choose would be 64150, that way we have 64101-64149 reserved future MessageOptions, and 64150-64199 reserved for FieldOptions.
There was a problem hiding this comment.
I see, thank you for the thorough explanation. I switched it to 64150.
Signed-off-by: Giedrius Statkevičius <[email protected]>
planetscale#140 requires >=1.23.0 Signed-off-by: Giedrius Statkevičius <[email protected]>
|
Trying to bump the Go version here: #141 |
Implement unmarshal_unique that is like unmarshal but calls unique.Make() on each string. The idea is to keep only one copy of a string around with gRPC marshaling/unmarshaling pools on.
No idea how to test this in unit tests but this change works in our project.
Fixes #139.