codec: implement codec v2

[jzelinskie]

This is a follow up from grpc/grpc-go#6619

Go's gRPC implementation added a new CodecV2 interface that enables integration with their memory pooling logic.
Adopting this should vastly improve performance and garbage collection overhead.

I took a stab at an implementation using the upstream version as reference.

In a review I'm looking for confirmation that this is the right approach for pooling and any feedback to make this more robust for a variety of vtprotobuf users. For example, I'm requiring SizeVT() here, which not everyone might generate; perhaps I could do a type assertion for that usage.

[mattrobenolt]

The downside I see to this implementation and the CodecV2 implementation in general is now the hard reliance on grpc the package since the interface relies on mem.BufferSlice.

This is kinda not ideal when using protobuf in contexts other than gRPC and use codecs for marshaling and unmarshalling. For example we also use vtprotobuf within ConnectRPC (https://connectrpc.com) which effectively is an alternative to gRPC, so is kinda wonky to bring in a dependency on gRPC itself just to utilize this mem.BufferSlice.

It seems in general more appropriate to me if this BufferSlice were an interface, or a part of the core protobuf package rather than grpc itself since codecs aren't specific to gRPC and are often used outside of it even outside the context of RPC at all.

Prior to this, vtprotobuf has no runtime dependency on the grpc package, it's only used within the code generator, so this change makes that an explicit runtime dependency which is awkward if you dont' even use grpc.

[jzelinskie]

I've typically vendored this Codec override into my codebase, but I'm PRing here for review. Maybe this could just be in an examples directory and not directly in vtprotobuf to avoid the dependency?

[coxley]

@jzelinskie: I don't personally have a strong opinion, but the example approach would fit given that's what is done for supporting both vtproto and standard in a single codec.

[coxley]

Also, while someone can generate only SizeVT(), they cannot use MarshalVT() without it. The generated marshaling code assumes size exists.

[vmg]

Cool! First off, thanks @jzelinskie for taking the time to contribute and review this with us. @mattrobenolt's concern is certainly valid, but as you've already pointed out, the pattern we use for these kind of adapters is merging them as examples as to not bring specific dependencies (GRPC in this case) to the project. We've done something similar for DRPC and found it generally useful without requiring to bring DRPC as a dependency.

Now, for those following at home, the new buffers feature that shipped in GRPC 1.66 actually has a performance regression that fortunately @coxley has figured out and fixed (grpc/grpc-go#7571). So thanks for that Codey! The fix hasn't been merged yet, which means that we can discuss the implementation of this side of the codec but running any actual benchmarks will be pointless until the fix lands and a new minor release is tagged.

As for the implementation itself: I think @na--'s comments are on point. MarshalToSizedBufferVT seems like the right API to use here, particularly since it handles the case of returning empty buffers in the same way as the previous codec and prevents the double-calculation of SizeVT (important!).

Besides those changes, we need to look at benchmarks before we can discuss further. I honestly have no idea of whether this will be a performance improvement at all! I think the killer feature here would be having a separate codegen step that allows serializing directly into mem.BufferSlice, because that could take very large messages (e.g. messages that have very large []byte or string fields) and include those oversized parts of the message as individual buffers in the buffer slice, instead of having to allocate a massive single buffer and copying the large []bytes into it. Of course, that comes with its own nightmare of complexities and lifetimes which makes it a non-trivial problem, and I'm not sure whether that's something that can be generalized with a codegen step.

In conclusion, my plan here is as follows:

1. Wait for GRPC 1.66.1
2. Try using this codec in Vitess itself and measure performance impact.
3. If the codec is faster, try a manual implementation of "large []byte fields from ProtoBuf get passed directly as part of the BufferSlice for Vitess' MySQL packet ProtoBufs (which are essentially just a bunch of large []byte).
4. If manually doing this kind of serialization has a measurable effect in benchmarks, try making a codegen phase for vtprotobuf that does just that for any oversized byte fields it finds during serialization.

I'll keep y'all posted!

[coxley]

@vmg Something that may be neat is finally being able to use pooled vtproto objects for unary client and servers.

It’d be a bit wonky — it would rely on a custom BufferPool. If the pool and codec could communicate, the uintptr (or weak.Pointer in 1.24) for a pooled slice could be mapped to a vtproto so that on pool.Put, we know we can release it.

May be more indirection than is worth it, but the pool interface at least makes it possible to derive the lifetime from gRPCs perspective.

[vmg]

Now testing vitessio/vitess#16790 in Vitess itself. Hoping this will show up in the benchmarks!

[coufalja]

There is a bug on this line IMO and it should go like either make([]byte, size) or mem.BufferSlice{mem.SliceBuffer(buf[:size])} on the line 35. Because the original slice reference will have its len still set to 0 effectively returning 0 length byte array from the method.

[jzelinskie]

Yes! Apologies -- I have this fixed in my PR for SpiceDB but didn't carry over the changes here.

[vmg]

This has worked wonderfully in upstream Vitess: vitessio/vitess#16790 (comment)