-
Notifications
You must be signed in to change notification settings - Fork 193
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 init time by switching to regex-lite #3269
Conversation
Whether or not the caching added in efd5032 is justified is open for debate. The following are client creation times on MacOS with and without it. With caching:
Without caching:
The first client construction with caching is consistently about 50µs slower than without. |
Overall, this is an improvement over what's currently released to crates.io (again, measured on MacOS):
The remaining large time chunks are being spent in default HTTP client initialization, which will be scrutinized in a separate PR. Same measurements in Amazon Linux 2 x64 on a c5.2xlarge EC2 instance:
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
After more profiling, the remainder of the overhead is almost entirely the TLS handshake on first request on Linux. On Mac, loading trusted certs is slow. I think if we were to make the default HTTP client lazy, we could consider only doing it for Mac. |
Ran the previous release benchmark to make sure this didn't regress anything significantly:
It seems to have increased request overhead by roughly 1 µs. |
do you want to target main or the release branch? |
"DEFAULT_PARTITION_RESOLVER" to RuntimeType.forInlineFun("DEFAULT_PARTITION_RESOLVER", EndpointStdLib) { | ||
rustTemplate( | ||
""" | ||
// Loading the partition JSON is expensive since it involves many regex compilations, | ||
// so cache the result so that it only need to be paid for the first constructed client. | ||
pub(crate) static DEFAULT_PARTITION_RESOLVER: #{Lazy}<#{PartitionResolver}> = | ||
#{Lazy}::new(|| #{PartitionResolver}::new_from_json(b$json).expect("valid JSON")); | ||
""", |
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.
we do eventually need to support runtime-loading of partitions.json but since this is all private, that seems fine
@@ -299,6 +299,7 @@ data class RuntimeType(val path: String, val dependency: RustDependency? = null) | |||
val PercentEncoding = CargoDependency.PercentEncoding.toType() | |||
val PrettyAssertions = CargoDependency.PrettyAssertions.toType() | |||
val Regex = CargoDependency.Regex.toType() | |||
val RegexLite = CargoDependency.RegexLite.toType() |
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.
actually unused, afaict
text.chars() | ||
// Filter out consecutive spaces | ||
.zip(text.chars().skip(1).chain(std::iter::once('!'))) | ||
.filter(|(a, b)| *a != ' ' || *b != ' ') | ||
.map(|(a, _)| a) | ||
.collect(), |
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.
clever algorithm!
a364d09
to
a365675
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
Each client initialization was taking between 1 and 2 milliseconds, regardless if the client had been constructed before or not. For example, if a customer wants five clients with different credentials providers, that could be 10 milliseconds of time spent in
Client::from_conf
. Approximately 98% of this time was spent compiling regular expressions for the endpoint partition resolver.This change switches everything over to the regex-lite crate, which has faster regex compile times, and shouldn't have much of an impact on performance for our specific use-cases (small strings, only evaluated at client initialization).
The use of regex was entirely removed in aws-sigv4 since it was overkill for what it was being used for.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.