stepancheg / grpc-rust

Rust implementation of gRPC
MIT License
1.37k stars 124 forks source link

MethodDescriptor seems suboptimal. #143

Closed tmccombs closed 5 years ago

tmccombs commented 5 years ago

The definition of MethodDescriptor seems suboptimal to me. It is:

pub struct MethodDescriptor<Req, Resp> {
    pub name: String,
    pub streaming: GrpcStreaming,
    pub req_marshaller: Box<Marshaller<Req> + Sync + Send>,
    pub resp_marshaller: Box<Marshaller<Resp> + Sync + Send>,
}

Note that name, req_marshaller, and resp_marshaller all use heap allocated memory. However, in almost all cases name could be a &'static str, and in most cases the marshallers are just grpc::marshaller::MarshallerProtobuf (is there a case where this wouldn't be true?).

I wonder if it would be better to have something like:

pub trait MethodDescriptor<Req, Resp> {
  fn name(&self) -> &str;
  fn streaming(&self) -> GrpcStreaming;
  fn write_req(&self, r: &Req) -> Result<Vec<u8>>;
  fn read_req(&self, bytes: Bytes) -> Result<Req>;
  fn write_resp(&self, r: &Resp) -> Result<Vec<u8>>;
  fn read_resp(&self, bytes: Bytes) -> Result<Resp>;
}

struct ProtobufMethodDescriptor<Req: Message, Resp: Message> {
  pub name: &'static str;
  pub streaming: GrpcStreaming;
}

impl<Req: Message, Resp: Message> MethodDescriptor<Req, Resp> for ProtobufMethodDescriptor<Req, Resp> {
  fn name(&self) -> &str { self.name }
  fn streaming(&self) -> GrpcStreaming { self.streaming }
  fn write_req(&self, r: &Req) -> Result<Vec<u8>> { MarshallerProtobuf.write(r) }
  fn read_req(&self, bytes: Bytes) -> Result<Req> { MarshallerProtobuf.read(bytes) }
  fn write_req(&self, r: &Resp) -> Result<Vec<u8>> { MarshallerProtobuf.write(r) }
  fn read_req(&self, bytes: Bytes) -> Result<Resp> { MarshallerProtobuf.read(bytes) }
}

Then use Arc<dyn MethodDescriptor<Req, Resp>> in places that currently use Arc<MethodDescriptor<Req, Resp>>.

Does that seem reasonable, or am I overlooking something?

tmccombs commented 5 years ago

Also somewhat related. Code generation generates that creates new method descriptors each time a new ClientStub is created, rather than creating the method descriptors once and cloning the Arcs when additional clients are created.

stepancheg commented 5 years ago

Fixed in master