Skip to content
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

Improve client interceptors chaining #4568

Conversation

amenzhinsky
Copy link
Contributor

@amenzhinsky amenzhinsky commented Jun 28, 2021

Continuation of #4524.

RELEASE NOTES:

client: improve performance when multiple interceptors are used

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for doing this! One small nit, and can you please add benchmarking/the results of the benchmarking similar to #4524?

clientconn.go Outdated
}
return func(ctx context.Context, method string, req, reply interface{}, cc *ClientConn, opts ...CallOption) error {
return interceptors[curr+1](ctx, method, req, reply, cc, getChainUnaryInvoker(interceptors, curr+1, finalInvoker), opts...)
func chainManyUnaryClientInterceptors(interceptors []UnaryClientInterceptor) UnaryClientInterceptor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Drop "Many" from these function names. GoLang style leans toward Brevity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to come up with a right name since all function names like chain(Client|Server)(Unary|Stream)Interceptors are already in use and chain(Unary|Stream)Interceptors belonged to the server side, so to avoid confusion I simply inlined multi-interceptor chaining in all related functions and replaced else if branches with switch cases.

@zasweq zasweq added this to the 1.40 Release milestone Jun 29, 2021
@amenzhinsky amenzhinsky force-pushed the improve-client-inverceptors-chaining branch from 8e72326 to c6c6f23 Compare June 30, 2021 17:01
@zasweq
Copy link
Contributor

zasweq commented Jun 30, 2021

Can you please provide some benchmarks for this change for the Client Side interceptors?

@amenzhinsky amenzhinsky force-pushed the improve-client-inverceptors-chaining branch from c6c6f23 to b5c31a2 Compare July 1, 2021 08:35
@amenzhinsky
Copy link
Contributor Author

Added benchmaks and here's the result:

benchmark                                        old ns/op     new ns/op     delta
BenchmarkChainClientUnaryInterceptors/1-12       7.68          7.69          +0.10%
BenchmarkChainClientUnaryInterceptors/3-12       146           151           +3.77%
BenchmarkChainClientUnaryInterceptors/5-12       318           182           -42.69%
BenchmarkChainClientUnaryInterceptors/10-12      767           222           -71.07%
BenchmarkChainClientStreamInterceptors/1-12      7.02          6.95          -1.07%
BenchmarkChainClientStreamInterceptors/3-12      133           164           +23.44%
BenchmarkChainClientStreamInterceptors/5-12      276           180           -34.84%
BenchmarkChainClientStreamInterceptors/10-12     551           209           -62.16%

benchmark                                        old allocs     new allocs     delta
BenchmarkChainClientUnaryInterceptors/1-12       0              0              +0.00%
BenchmarkChainClientUnaryInterceptors/3-12       2              3              +50.00%
BenchmarkChainClientUnaryInterceptors/5-12       4              3              -25.00%
BenchmarkChainClientUnaryInterceptors/10-12      9              3              -66.67%
BenchmarkChainClientStreamInterceptors/1-12      0              0              +0.00%
BenchmarkChainClientStreamInterceptors/3-12      2              3              +50.00%
BenchmarkChainClientStreamInterceptors/5-12      4              3              -25.00%
BenchmarkChainClientStreamInterceptors/10-12     9              3              -66.67%

benchmark                                        old bytes     new bytes     delta
BenchmarkChainClientUnaryInterceptors/1-12       0             0             +0.00%
BenchmarkChainClientUnaryInterceptors/3-12       96            80            -16.67%
BenchmarkChainClientUnaryInterceptors/5-12       192           80            -58.33%
BenchmarkChainClientUnaryInterceptors/10-12      432           80            -81.48%
BenchmarkChainClientStreamInterceptors/1-12      0             0             +0.00%
BenchmarkChainClientStreamInterceptors/3-12      96            80            -16.67%
BenchmarkChainClientStreamInterceptors/5-12      192           80            -58.33%
BenchmarkChainClientStreamInterceptors/10-12     432           80            -81.48%

@zasweq
Copy link
Contributor

zasweq commented Jul 2, 2021

Hey, the benchmarks show a positive increase. Sorry, but the team can't take the PR with all the positive increases. The Server Side one was fantastic though :).

@zasweq zasweq closed this Jul 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants