transport: performance improvements #588
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
CLAs look good, thanks! |
| func (windowUpdate) isItem() bool { | ||
| return true | ||
| } | ||
| func (*windowUpdate) isItem() {} |
There was a problem hiding this comment.
why is this changed to the pointer receiver?
There was a problem hiding this comment.
See the commit message https://github.com/tamird/grpc-go/commit/7e7145d85838627ba8531dc2f77623ef1a5b573a
There was a problem hiding this comment.
But you would have to adjust the call sites too, which you don't. So this change is does not even seem correct.
There was a problem hiding this comment.
Sorry, what?
As explained in the commit message, value receiver interface
implementations implement the interface for both value and pointer. In this
case, all the existing instances already use pointers, which is why there
is no fallout.
On Mar 7, 2016 16:36, "Brad Fitzpatrick" notifications@github.com wrote:
In transport/control.go
#588 (comment):@@ -56,43 +56,33 @@ type windowUpdate struct {
increment uint32
}-func (windowUpdate) isItem() bool {
- return true
-}
+func (*windowUpdate) isItem() {}But you would have to adjust the call sites too, which you don't. So this
change is does not even seem correct.—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc-go/pull/588/files#r55276401.
There was a problem hiding this comment.
Oh, I see. Both are already only used as pointers. Weird.
I'll defer to @iamqizhao. I agree that it's weird having the receiver type here be values, but it's also weird that the code is passing around pointers for these. I think for code clarity reasons I'd prefer to just change them all to be values. As a small bonus, putting an empty struct value like flushIO into an interface value is free, and doesn't require allocations like it is now:
transport/http2_client.go: t.controlBuf.put(&flushIO{})
transport/http2_client.go: case *flushIO:
transport/http2_server.go: t.controlBuf.put(&flushIO{})
transport/http2_server.go: case *flushIO:
Changing it to pointers makes it look like it has mutable state.
There was a problem hiding this comment.
I think this is better as-is. Unfortunately you lose too much type safety when you implement interfaces with values in Go.
There was a problem hiding this comment.
Unfortunately you lose too much type safety when you implement interfaces with values in Go.
I don't understand this comment. Can you give an example?
There was a problem hiding this comment.
|
@iamqizhao ping |
|
what about #587? Should it be closed or merge? |
|
Closed #587. Thoughts on this PR? |
|
I object to this CL for the same reasons listed in #587 (comment) Please break this up so they can be reviewed independently. |
This provides a modest reduction in allocation count. Benchmarked using https://github.com/cockroachdb/rpc-bench: ``` name old time/op new time/op delta GRPCServeHTTP_1K-4 181µs ± 3% 178µs ± 2% ~ (p=0.310 n=5+5) GRPCServeHTTP_64K-4 1.31ms ± 4% 1.30ms ± 3% ~ (p=0.548 n=5+5) name old speed new speed delta GRPCServeHTTP_1K-4 11.3MB/s ± 3% 11.5MB/s ± 2% ~ (p=0.246 n=5+5) GRPCServeHTTP_64K-4 100MB/s ± 4% 100MB/s ± 2% ~ (p=0.548 n=5+5) name old alloc/op new alloc/op delta GRPCServeHTTP_1K-4 88.5kB ± 0% 88.5kB ± 0% -0.09% (p=0.008 n=5+5) GRPCServeHTTP_64K-4 801kB ± 0% 801kB ± 0% -0.05% (p=0.008 n=5+5) name old allocs/op new allocs/op delta GRPCServeHTTP_1K-4 167 ± 0% 162 ± 0% -2.99% (p=0.008 n=5+5) GRPCServeHTTP_64K-4 672 ± 0% 645 ± 0% -4.08% (p=0.008 n=5+5) ```
|
OK, this is just one commit now. |
|
Okay, @iamqizhao can review this one. I have no opinion on it. |
Like #587 but without the last commit.
Updates #586.
@bradfitz