-
Notifications
You must be signed in to change notification settings - Fork 716
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
fix(bindings): ConfigPool should always yield associated connections #4708
Conversation
@@ -0,0 +1,44 @@ | |||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
// SPDX-License-Identifier: Apache-2.0 | |||
|
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.
If you didn't already, it's probably a good idea to re-run this with a more recent allocator (e.g., jemalloc) and see how much that changes the results. Still, ~2 microsecond of difference feels to me like there is probably no reason to use a connection pool -- maybe we have plans that suggest we can get more out of pooling than we do today in this benchmark?
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.
Rerunning the benchmarks with jemalloc I got the following results.
Connection Creation/connection reuse
time: [751.49 ns 751.64 ns 751.78 ns]
change: [-3.3691% -3.3445% -3.3186%] (p = 0.00 < 0.05)
Performance has improved.
Connection Creation/connection allocation
time: [2.2163 µs 2.2170 µs 2.2178 µs]
change: [-23.654% -23.616% -23.581%] (p = 0.00 < 0.05)
Performance has improved.
Essentially jemalloc improved the connection allocation/free performance by ~23%.
I agree that ~2 microseconds is a relatively small improvement.
maybe we have plans that suggest we can get more out of pooling than we do today in this benchmark?
I'm not aware of any, although we're certainly open to suggestions! My goal with this benchmark was to have some empirical data to help highlight the benefit or lack thereof.
My understanding is that connection reuse was one of the older features of s2n-tls, so perhaps the performance benefit was more significant with older platforms/allocators?
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.
Connection reuse is specifically intended for situations where allocation is expensive. There would be no reason to reuse connections with a modern allocator like jemalloc. Unfortunately most of our customers don't use jemalloc ;)
- change comment wording to call out benchmark as simple - note that this is intended for environments where allocation are unusually expensive.
- move documentation to module - remove concrete numbers
- rustfmt
Description of changes:
always reset the config on connections yielded from ConfigPool to account for Callbacks that might be swapping the config on connections.
This also adds a doc comment for the ConfigPool confirming that it is a connection pool, and adds a benchmark to get real numbers about the performance savings of connection reuse.
This change degrades ConfigPool performance by about 4%
(772 ns -> 804 ns).
Testing:
Added a unit test to prevent regression on this behavior.
And added a benchmark to understand the performance of the ConfigPool.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.