-
Notifications
You must be signed in to change notification settings - Fork 749
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
Major client performance refactor #489
Conversation
JamesNK
commented
Sep 4, 2019
- Cache Uri and call scope for methods on channel
- Hardcode GrpcCall logger name to avoid reflection
- Reduce async state machines allocated
- Switch from Task to ValueTask where possible
- Reuse System.Version
- Optimize content-type validation
- Optimize fields on GrpcCall
- Create PushUnaryContent and PushStreamContent
Before:
After:
|
2bede53
to
3bb6e62
Compare
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.
I skimmed through the PR and generally looks ok, but I'd still like @JunTaoLuo to give a final LGTM after reviewing more in detail.
@@ -40,6 +37,9 @@ public sealed class GrpcChannel : ChannelBase, IDisposable | |||
{ | |||
internal const int DefaultMaxReceiveMessageSize = 1024 * 1024 * 4; // 4 MB | |||
|
|||
private readonly ConcurrentDictionary<IMethod, GrpcMethodInfo> _methodInfoCache; |
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.
not sure I recall this correctly, but the concurrentDictonary tends to allocate even in situations when one doesn't expect that, so if the purpose of the cache is to save allocations of new GrpcMethodInfo instances, then this might only be a partial solution.
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.
Could you be more specific?
I cached the delegate used with GetOrAdd in a field _createMethodInfoFunc
so it is allocated once per channel, rather than once per call. Other than that I didn't notice additional allocations.
3bb6e62
to
f62c1a0
Compare
f62c1a0
to
71abe67
Compare
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.
Still digesting some of the GrpcCall changes. Will finish the review tomorrow.
test/Grpc.Net.Client.Tests/HttpContentClientStreamReaderTests.cs
Outdated
Show resolved
Hide resolved
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.
Looks clean!
71abe67
to
535ada4
Compare