Remove call to proto.Clone() in http2Server.WriteStatus. #2842

easwars · 2019-05-30T19:44:22Z

Expose a method from the internal package to get to the raw
StatusProto wrapped by the status error, and use it from
http2Server.WriteStatus().
Add a helper method in internal/testutils to compare two status errors
and update test code to use that instead of reflect.DeepEqual()

Update fork with changes from master

Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there.

I need to roll these back because I had sent out changes from my master branch, and this is causing a lot of pain right now for my work on other branches. I will create a branch for this issue and will send out these changes in a fresh PR.

Sync fork with upstream master.

dfawley · 2019-05-30T19:59:30Z

The go.mod and go.sum changes should be reverted from this PR.
Consider squashing all your commits down to 1 (or a small number if there are relevant discrete changes for one PR) before sending the first iteration of a PR. You can do this now and then git push -f your branch.
For best results, use git fetch upstream master && git rebase upstream/master to pull mainline commits into your branch when needed instead of using git merge.

…#2842.

* Expose a method from the internal package to get to the raw StatusProto wrapped by the status error, and use it from http2Server.WriteStatus(). * Add a helper method in internal/testutils to compare two status errors and update test code to use that instead of reflect.DeepEqual()

easwars · 2019-05-30T23:00:23Z

The go.mod and go.sum changes should be reverted from this PR.
This is done now.

Consider squashing all your commits down to 1 (or a small number if there are relevant discrete changes for one PR) before sending the first iteration of a PR. You can do this now and then git push -f your branch.
I think I have squashed my three commits made as part of this PR now. From next time, will do a better job.

For best results, use git fetch upstream master && git rebase upstream/master to pull mainline commits into your branch when needed instead of using git merge.
I'm guessing, this is for the future.

Thanks for the git tips. Definitely going to be useful moving forward.

easwars · 2019-05-30T23:43:09Z

The go.mod and go.sum changes should be reverted from this PR.

This is done now.

vet is failing because the reverted go.sum and go.mod changes? How do we usually handle this?

Consider squashing all your commits down to 1 (or a small number if there are relevant discrete changes for one PR) before sending the first iteration of a PR. You can do this now and then git push -f your branch.

I think I have squashed my three commits made as part of this PR now. From next time, will do a better job.

For best results, use git fetch upstream master && git rebase upstream/master to pull mainline commits into your branch when needed instead of using git merge.

I'm guessing, this is for the future.

Thanks for the git tips. Definitely going to be useful moving forward.

dfawley

LGTM modulo the one small change requested.

dfawley · 2019-05-31T17:50:43Z

internal/transport/http2_server.go

@@ -817,7 +819,8 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error {
 	headerFields = append(headerFields, hpack.HeaderField{Name: "grpc-status", Value: strconv.Itoa(int(st.Code()))})
 	headerFields = append(headerFields, hpack.HeaderField{Name: "grpc-message", Value: encodeGrpcMessage(st.Message())})

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


It should be (a tiny bit) better to declare this globally and avoid the type assertion every time status is written:

var statusRawProto = internal.StatusRawProto.(func(*status.Status) *spb.Status)

(Also it would fail at init time if the types are wrong, instead of runtime, which is also a win.)

Done. Thanks.

dfawley · 2019-06-03T19:29:37Z

I also meant to request some performance metrics for this change - can you run a QPS-style workload (many concurrent, small RPCs) before/after and see if this makes much difference in performance / allocations? Benchmark results should be posted in the first PR comment. Thanks.

easwars · 2019-06-03T20:40:45Z

I ran a benchmark with 100 concurrent calls and req and resp sizes of 32 bytes. There doesn't seem to any significant change in performance.

Stream-traceMode_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_100-reqSize_32B-respSize_32B-Compressor_off-Preloader_false
Title Before After Percentage
Bytes/op 1345 1346 0.07%
Allocs/op 32 32 0.00%
50 latency 223.788µs 227.003µs 1.44%
90 latency 288.836µs 290.117µs 0.44%

dfawley · 2019-06-04T21:54:24Z

Interesting. Do we trust the allocation numbers? I would expect them to be lower (both count and size) since we are removing proto.Clone calls. Though the status protos are very small without any "details" additions, so I would expect very small gains. Would you mind modifying the benchmarks locally to include a large details message in the server's response status to see if we can coerce a noticeable performance delta out of the benchmarks?

easwars · 2019-06-04T23:27:47Z

I made a change to the server to return a status with details message.

When I used a details message of len 128, this was the result:
Title Before After Percentage
Bytes/op 14275 13979 -2.07%
Allocs/op 170 166 -2.35%

And when I used a details message of len 512, this was the result:
Title Before After Percentage
Bytes/op 17961 17281 -3.79%
Allocs/op 172 168 -2.33%

The allocation numbers seem to be reasonably trustworthy in the sense that the number of allocs/op has not changed based on the length of the details message while the bytes/op has (and is proportional in some sense to the length of the details message).

easwars · 2019-06-05T22:00:58Z

@dfawley : Is there a way to get the vet test to be happy without the go.mod and go.sum changes?

dfawley · 2019-06-06T20:22:53Z

internal/testutils/status_equal.go

+package testutils
+
+import (
+	"github.com/gogo/protobuf/proto"


This should not use gogo/proto

Done. Thanks.
Some of the tests are unusually flaky today. Unable to make travis happy as of now.

easwars added 12 commits April 2, 2019 13:36

Add full support for configuring exponential connection backoff params.

36ed5dd

Add full support for configuring exponential connection backoff params.

dffc6d6

Merge branch 'master' of github.com:easwars/grpc-go

a92e19d

Merge pull request #1 from grpc/master

e51c0c3

Update fork with changes from master

Handle review comments.

65e8212

Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there.

Handle vet failure.

44d0c79

Rollback changes made as part of grpc#2735.

8ffe677

I need to roll these back because I had sent out changes from my master branch, and this is causing a lot of pain right now for my work on other branches. I will create a branch for this issue and will send out these changes in a fresh PR.

Missed this in the previous commit.

1c29ad5

Merge pull request #2 from grpc/master

f5d99a4

Sync fork with upstream master.

Unexpected diff from upstream.

57e1ed6

Merge remote-tracking branch 'upstream/master'

8b06ff0

Merge remote-tracking branch 'upstream/master'

3e49ea2

easwars requested review from menghanl and dfawley May 30, 2019 19:45

easwars added a commit to easwars/grpc-go that referenced this pull request May 30, 2019

Revert changes in go.sum and go.mod as per review comments in PR grpc…

61014c4

…#2842.

easwars force-pushed the status branch from 61014c4 to 57b05be Compare May 30, 2019 22:59

dfawley approved these changes May 31, 2019

View reviewed changes

Review comments.

e2bdc77

dfawley reviewed Jun 6, 2019

View reviewed changes

Do not use gogo/proto for proto.Clone()

a775841

easwars merged commit a5396fd into grpc:master Jun 10, 2019

dfawley added the Type: Performance Performance improvements (CPU, network, memory, etc) label Jun 13, 2019

dfawley added this to the 1.22 Release milestone Jun 13, 2019

thaJeztah mentioned this pull request Aug 26, 2019

bump google.golang.org/grpc v1.23.0 (CVE-2019-9512, CVE-2019-9514, CVE-2019-9515) containerd/containerd#3581

Merged

easwars deleted the status branch November 20, 2019 17:11

lock bot locked as resolved and limited conversation to collaborators May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove call to proto.Clone() in http2Server.WriteStatus. #2842

Remove call to proto.Clone() in http2Server.WriteStatus. #2842

easwars commented May 30, 2019

dfawley commented May 30, 2019

easwars commented May 30, 2019

easwars commented May 30, 2019

dfawley left a comment

dfawley May 31, 2019

easwars Jun 3, 2019

dfawley commented Jun 3, 2019

easwars commented Jun 3, 2019

dfawley commented Jun 4, 2019

easwars commented Jun 4, 2019

easwars commented Jun 5, 2019

dfawley Jun 6, 2019

easwars Jun 10, 2019

Remove call to proto.Clone() in http2Server.WriteStatus. #2842

Remove call to proto.Clone() in http2Server.WriteStatus. #2842

Conversation

easwars commented May 30, 2019

dfawley commented May 30, 2019

easwars commented May 30, 2019

easwars commented May 30, 2019

dfawley left a comment

Choose a reason for hiding this comment

dfawley May 31, 2019

Choose a reason for hiding this comment

easwars Jun 3, 2019

Choose a reason for hiding this comment

dfawley commented Jun 3, 2019

easwars commented Jun 3, 2019

dfawley commented Jun 4, 2019

easwars commented Jun 4, 2019

easwars commented Jun 5, 2019

dfawley Jun 6, 2019

Choose a reason for hiding this comment

easwars Jun 10, 2019

Choose a reason for hiding this comment