search

twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.06k stars 328 forks source link

Integration with vtprotobuf #327

Closed T-J-L closed 2 years ago

T-J-L commented 3 years ago

I’m currently using twirp and have been exploring vtprotobuf to generate optimized Marshal /Unmarshal methods for proto messages.

To modify a twirp file to use vtproto is simple. An example code change would be to change...

respBytes, err := proto.Marshal(respContent)

to...

respBytes, err := respContent.MarshalVT()

The equivalent can be done with Unmarshal. I've currently done this with a find and replace, with good results.

Is integration with vtprotobuf something that could be considered as a code generation option? If so, I’m happy to make the changes.

3ventic commented 3 years ago

We generally try to avoid adding additional options or dependencies unless there's a compelling reason to do it. It sounds like a separate script to replace the calls in the Twirp-generated code would already accomplish this without issues; is that correct based on your testing with it?

T-J-L commented 3 years ago

That’s understandable. The find and replace option only applies to the server code, due to the generic marshalling code in the client. I had some time, so I’ve forked and applied the changes for both client and server code generation if you’re interested here.

I’ll run benchmarks next week for my own workload but I expect it’s not worth maintaining a fork long-term.

T-J-L commented 3 years ago

Just to close this off, the performance benchmarks were significant, reducing all serialization time to about 40-50% of the reflection-based option. This is now up and running in production.

Just want to also point to https://github.com/twitchtv/twirp/issues/261 which I found; this solution would be a generic way of allowing users to implement custom marshalling.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label / comment or this will be closed in 5 days