grpc-dotnet icon indicating copy to clipboard operation
grpc-dotnet copied to clipboard

Proposal: allow memory-based Metadata.Entry

Open mgravell opened this issue 1 year ago • 2 comments

Currently Metadata.Entry has a string and byte[] field. These must be right-sized, and do not allow leased (oversized) buffers, etc. For this scenario, I propose:

  • change the fields to be object payload, int offset, int length
  • existing constructors would set offset 0 and length .Length
  • add constructors taking ReadOnlyMemory<byte> and ReadOnlyMemory<char>
  • the payload object could be a string, char[], byte[], or a memory manager (byte or char) as extracted via MemoryMarshal - in all cases count and offset also need to be respected
  • enhance type test for the IsBinary check
  • add accessor for the ReadOnlyMemory<> pair, using switch (payload) type-test
  • existing accessors should alloc if needed and update their fields to the new right-size copy, so repeated calls to the accessors only allows at most once
  • change existing usage in managed transport to use the new accessors exclusively (for no additional allocs)
  • optional: change the existing usage in the unmanaged transport similarly
  • consider (not definite) marking the old accessors as obsolete, advising people to prefer the new accessors
  • optional: consider using leased buffers for the lifetime of received aspnet headers, although we don't strictly control whether these values could leak into unknown code, so maybe not (or maybe an option?); could be achieved using at most one byte[] and at most one char[] lease per request, by sizing the lease for all headers and slicing accordingly

Outcome:

  • existing code all still works
  • no change to object size (on x64), by swapping two refs for one ref and two integers
  • capability to use alloc-free string substring ranges, leased buffers, custom memory, etc

I'm happy to do the work, if this is something that would be considered

(it is a shame it isn't a struct, but that ship has well and truly sailed)

mgravell avatar Mar 19 '23 13:03 mgravell