-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transport: allocate bigger slabs and reuse #587
Conversation
Updates #586. |
@iamqizhao, I am against this change for a number of reasons:
I think 1) alone is enough to reject this request. The pieces can be submitted separately for independent review, but it's too much distraction trying to review them together. |
I agree. I simply had no idea what is going on with this PR. I remember you had some comments in this PR but could not find them any more. |
My comments were deleted when this pull request was updated. Because Github totally sucks for code review. I think we should switch gRPC 100% to Gerrit at some point. |
Some of this PR was extracted into #588, which should be more digestible.
|
Closing this for now. |
This provides a substantial speed improvement with large payloads. Benchmarked using https://github.com/cockroachdb/rpc-bench: ``` name old time/op new time/op delta GRPCServeHTTP_1K-4 178µs ± 2% 177µs ± 1% ~ (p=1.000 n=5+5) GRPCServeHTTP_64K-4 1.30ms ± 3% 1.09ms ± 4% -16.82% (p=0.008 n=5+5) name old speed new speed delta GRPCServeHTTP_1K-4 11.5MB/s ± 2% 11.6MB/s ± 1% ~ (p=1.000 n=5+5) GRPCServeHTTP_64K-4 100MB/s ± 2% 121MB/s ± 4% +20.26% (p=0.008 n=5+5) name old alloc/op new alloc/op delta GRPCServeHTTP_1K-4 88.5kB ± 0% 93.9kB ± 0% +6.16% (p=0.008 n=5+5) GRPCServeHTTP_64K-4 801kB ± 0% 791kB ± 0% -1.16% (p=0.008 n=5+5) name old allocs/op new allocs/op delta GRPCServeHTTP_1K-4 162 ± 0% 156 ± 0% -3.70% (p=0.008 n=5+5) GRPCServeHTTP_64K-4 645 ± 0% 284 ± 0% -55.96% (p=0.016 n=5+4) ```
Updated; just one commit here now. |
LGTM. @iamqizhao? I still want to delete all this code, but this is fine and better for now. |
@iamqizhao ping |
The PR's description is out-dated. need either update. |
Huh? Looks up-to-date to me. |
} | ||
if err != nil { | ||
s.buf.put(&recvMsg{err: mapRecvMsgError(err)}) | ||
return | ||
} | ||
if len(buf) == 0 { | ||
buf = make([]byte, readSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? the next iteration will do anyways (line 319).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the loop on line 319 only runs buf := make(...)
once, and each iteration exhausts some of the front of buf until it's gone. This refills it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, I misread.
@iamqizhao ping. |
This provides a substantial speed improvement with large payloads.
Benchmarked using https://github.com/cockroachdb/rpc-bench.
@iamqizhao @bradfitz