Wasted proto.Clone in http2Server.WriteStatus

[coocood]

grpc-go/transport/http2_server.go

Line 773 in 7268ca4

if p := st.Proto(); p != nil && len(p.Details) > 0 {

Most of the time, there is no detail in the status.
In our application, the proto.Clone itself takes about 2% of total CPU time.
It can be avoided if we add a method HasDetail in Status.

[dfawley]

I think we could move some of the implementation of status into internal/ and play tricks to allow ourselves to efficiently access the proto when we know it won't be changing, but still force external users to use it immutably.

[easwars]

@dfawley

Would it make sense to add the following two methods to status.Status ?

func (s *Status) MarshalledProto() ([]byte, err) {...}
func (s *Status) HasDetails() bool {...}

And WriteStatus in http2_server.go could directly get the marshalled bytes if the status has details.

[dfawley]

That technically works but it complicates the API for something that external users probably won't ever want.

Another trick we could play:

package internal;

var StatusRawProto func(*status.Status) *spb.Status

---

package status;

func init() {
  internal.StatusRawProto = func(s *Status) *spb.Status { return s.s }
}

---

package transport;

  ...
  if p := internal.StatusRawProto(st); p != nil && len(p.Details) > 0 {
  ...

[easwars]

Thanks. Makes sense.

But, what is the advantage (or need) of defining StatusRawProto as a var in the internal package and have it initialized in init(), instead of just making it a func in the internal package? Would we override it somewhere?

[dfawley]

It can't be implemented in internal because it needs access to private members of the Status struct.

[easwars]

@dfawley

I tried making the following changes:

package internal;

var  StatusRawProto interface{} // func (*status.Status) *spb.Status
---

package status;

func init() {
       internal.StatusRawProto = statusRawProto
}

func statusRawProto(s *Status) *spb.Status { return s.s }
---

package transport;
  ...
  srp := internal.StatusRawProto.(func(*status.Status) *spb.Status)
  if p := srp(st); p != nil && len(p.Details) > 0 {
  ...

I see that a couple of tests which do a reflect.DeepEqual(gotErr, wantErr) are failing. The actual fields that we care about in the error are actually the same, but some internal proto fields are different.

I did a cmp.Diff(gotErr, wantErr) and got this:
=== RUN Test/FailedEmptyUnary
--- FAIL: Test (0.02s)
--- FAIL: Test/FailedEmptyUnary (0.00s)
end2end_test.go:580: Running test in tcp-clear-v1-balancer environment...
end2end_test.go:2892: diff is {*status.statusError}.Details[0].XXX_sizecache:
-: 0
+: 13
{*status.statusError}.XXX_sizecache:
-: 0
+: 51
end2end_test.go:2893: TestService/EmptyCall(_, _) = _, rpc error: code = DataLoss desc = error for testing: fail-this-RPC, want _, rpc error: code = DataLoss desc = error for testing: fail-this-RPC
FAIL

Does this mean that we cannot avoid a proto.Clone in the case where the error actually has Details and we need to Marshal it, or does the test need fixing?

Thanks.

[dfawley]

The tests should be fixed - they were just getting lucky before. It's incorrect to compare protos with a reflect.DeepEqual/equivalent. proto.Equal needs to be used instead. Might be worth making a helper (in internal/testutils?) to compare status errors since this is likely to be prevalent across tests in many packages.

[easwars]

I added a helper in internal/testutils and changed a bunch of places to not use reflect.DeepEqual, but when I got to status_test.go, I realized I had a cyclic dependency.

Would you be OK to add the helper to the status package itself?

// this will be useful when we have a Status object, or if we want to use cmp.Equal() at some point
func (s *Status) Equal(other *Status) bool {...} 

// this is for errors generated from Status
func ErrEqual(err1, err2 error) bool {...} 

[dfawley]

I'm OK with something like that, but only if it's critical for some use cases or is something many people will want to use as a convenience, since it clutters the API. How about let's leave it out for now, re-implement the equality checking in status_test.go instead of going through internal/testing, and let non-internal users use proto.Equal(status1.Proto(), status2.Proto()) until there is a strong need for something faster (avoiding the Clones) externally?