change objects in recvBuffer queue from interface to concrete type to reduce allocs

[apolcyn]

This removes the next largest remaining source of total allocs and appears to make a slight ~2.5-3% QPS improvement after #940, #1010, and #1028.

Based off of the server protobuf streaming QPS test memory profile with the combined changes, shown in #1028, the allocation in:

1342.06MB 35.49% 35.49%  1342.06MB 35.49%  google.golang.org/grpc/transport.(*Stream).write

looks to be from allocating recvMsg structs onto the heap and passing their reference onto the buffer in
https://github.com/grpc/grpc-go/blob/master/transport/transport.go#L316

Changing the channel recvBuffer to pass by the struct onto the buffer by value, on top of the combined changes in #940, #1010, and #1028, memory profile looks like:

(same testing environment as in #1028)

2475.08MB of 2501.20MB total (98.96%)
Dropped 88 nodes (cum <= 12.51MB)
      flat  flat%   sum%        cum   cum%
 1330.56MB 53.20% 53.20%  1330.56MB 53.20%  golang.org/x/net/http2.parseDataFrame
  463.51MB 18.53% 71.73%   928.51MB 37.12%  google.golang.org/grpc.encode
  456.01MB 18.23% 89.96%   456.01MB 18.23%  github.com/golang/protobuf/proto.(*Buffer).enc_len_thing
     214MB  8.56% 98.52%      214MB  8.56%  google.golang.org/grpc/transport.(*http2Server).handleData
       9MB  0.36% 98.88%   465.01MB 18.59%  google.golang.org/grpc.protoCodec.Marshal
       2MB  0.08% 98.96%   934.51MB 37.36%  google.golang.org/grpc.(*Server).processStreamingRPC

(total allocation goes from ~3.7GB to ~2.5GB)

QPS roughly goes from ~816K (with combined memory reduces so far) to ~840K on sequential runs.

[apolcyn]

FTR the remaining sources appear to be more difficult to remove:

for the rest of the alloc sources, it appears that:
1330.56MB 53.20% 53.20% 1330.56MB 53.20% golang.org/x/net/http2.parseDataFrame from within go http2 library, for new structs per data frame read.

463.51MB 18.53% 71.73% 928.51MB 37.12% google.golang.org/grpc.encode from https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L295

456.01MB 18.23% 89.96% 456.01MB 18.23% github.com/golang/protobuf/proto.(*Buffer).enc_len_thing undetermined but within proto library.

214MB 8.56% 98.52% 214MB 8.56% google.golang.org/grpc/transport.(*http2Server).handleData from https://github.com/grpc/grpc-go/blob/master/transport/http2_server.go#L415

9MB 0.36% 98.88% 465.01MB 18.59% google.golang.org/grpc.protoCodec.Marshal from new slices in https://github.com/grpc/grpc-go/pull/1010/files#diff-365652330e79ea705fe1e6d0a561ee13R65 and https://github.com/grpc/grpc-go/pull/1010/files#diff-365652330e79ea705fe1e6d0a561ee13R73

But profile with this shows ~15% cum. CPU clock in transport.(*recvBufferReader).Read and 22% in write syscalls.

[tamird]

Without intending to sound pedantic, the source of improvement here is the removal of the item interface, more than the change from pointer to value. If you had kept the item interface and changed recvMsg to implement it by value, you would not have observed a change in memory allocations.

The commit message and PR description should both be amended to reflect the true source of improvement here.

EDIT: Apologies, the item interface was not removed - but recvMsg no longer implements it. With that addendum, the above is still relevant.

[apolcyn]

@tamird thanks but I think the removal of the interface effect ends up having the same benefit of copy-by-ref to copy-by-value.

I was thrown off by this as changing the pure recvMsg{} struct to implement item interface was tried first, but I when looking at the memory profiler, the same total allocation that used to be in transport.(*Stream).write was just switched to the go runtime function that converts structs to interfaces (runtime.convT2E I think it was). Looked into this and from what I found converting a go struct to an interface causes a heap alloc of the struct and puts its pointer in the new interface object, so the problem of passing a pointer to alloc'd object was still there.

[tamird]

Yes, that's correct, and that's my point - simply changing &recvMsg{} to recvMsg{} without also specializing the recvBuffer to use the concrete type rather than the interface does not improve memory allocation for the reason you mention.

The bottom line: this improvement is due to the specialization of recvBuffer from item (an interface) to recvMsg (a concrete type).

[apolcyn]

I see, updated title

[tamird]

Cool, thanks. Please update the commit message as well.

[apolcyn]

done, thanks!

[dfawley]

@apolcyn what is the state of this PR?

[apolcyn]

Just updated, I think this should still be good.

cc @mehrdada for how this incorporates your recent change to transport.go

[mehrdada]

b.backlog[0] = recvMsg{} is more robust if we add fields to the struct in the future.

Also, do we ever close the channel in recvMsg?

[dfawley]

+1, assuming perf is not worse.

[apolcyn]

@mehrdada done for first comment, sorry for delay.

Also, do we ever close the channel in recvMsg?

I don't think we do explicitly close this channel, but I think it's ok for this channel, since the blocking read can unblock when the context is Done

[mehrdada]

LGTM. Thanks!

[dfawley]

I took your branch and rebased it (locally) and the tests still passed. Merging.