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

Client::from_config() takes several ms to build a client #975

Closed
monken opened this issue Nov 25, 2023 · 11 comments
Closed

Client::from_config() takes several ms to build a client #975

monken opened this issue Nov 25, 2023 · 11 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.

Comments

@monken
Copy link

monken commented Nov 25, 2023

Describe the bug

I need to create many clients with different credentials and endpoint parameters. I've been using aws_sdk_s3::Client::from_conf(config) for that. After looking into some performance issues, I realized that Client::from_conf(config) takes roughly 25ms in my dev build and 2.5ms in a release build. The config configures static credentials, a region and the identity cache is disabled with IdentityCache::no_cache().

I'm not able to track down where the slow path is. Any ideas how to speed this up?

Expected Behavior

Creating a client should take microseconds.

Current Behavior

Creating a client should take milliseconds.

Reproduction Steps

let start = std::time::Instant::now();
let client = Client::from_conf(config);
info!("Built AWS Client in {:?}", start.elapsed());

Possible Solution

No response

Additional Information/Context

No response

Version

aws-sdk-s3 v0.36.0
aws-runtime v0.57.2

Environment details (OS name and version, etc.)

Apple M2

Logs

No response

@monken monken added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 25, 2023
@monken
Copy link
Author

monken commented Nov 26, 2023

I believe it takes several ms to create the hyper_14 client with rustls. However, even if I create the client manually, the time does not improve:

        let tls_connector = hyper_rustls::HttpsConnectorBuilder::new()
            .with_webpki_roots()
            .https_only()
            .enable_http1()
            .enable_http2()
            .build();
        let http_client = HyperClientBuilder::new().build(tls_connector);

I passhttp_client.clone() to Config::builder() but the build time of the client does not improve.

@rcoh
Copy link
Contributor

rcoh commented Nov 27, 2023

yeah—the issue is creating the Hyper client. This is a priority to fix. Note that this should only occur once per process.

@rcoh rcoh added p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Nov 27, 2023
@rcoh rcoh self-assigned this Nov 27, 2023
@monken
Copy link
Author

monken commented Nov 27, 2023

Agreed, the only issue is that I need to create many clients with different credentials. I tried to pass in the client to the S3Client builder, but it seems to initialize or build another client anyways.

@rcoh
Copy link
Contributor

rcoh commented Nov 27, 2023

do you have a code snippet? the connector should be cached in a lazy static so only the first client should be slow to initialize

@monken
Copy link
Author

monken commented Nov 27, 2023

I create a config like so:

let tls_connector = hyper_rustls::HttpsConnectorBuilder::new()
    .with_webpki_roots()
    .https_only()
    .enable_http1()
    .build();
let http_client: aws_sdk_s3::config::SharedHttpClient = HyperClientBuilder::new().build(tls_connector);

let s3config = Config::builder()
    .identity_cache(IdentityCache::no_cache())
    .region(Region::new(region))
    .endpoint_url(endpoint)
    .http_client(http_client)
    .force_path_style(true);

And then I create an S3 client per per set of credentials:

let config = self
    .config
    .clone()
    .set_credentials_provider(Some(aws_credentials))
    .clone()
    .build();

start = std::time::Instant::now();
let client = Client::from_conf(config);
info!("Built AWS Client in {:?}", start.elapsed());

I assume the Lazy HTTPS_NATIVE_ROOTS have not been evaluated before I clone the config?

@rcoh
Copy link
Contributor

rcoh commented Nov 28, 2023

ah—gotcha. Yeah this is from the bug where the native-tls client is always instantiated. We're working on a fix.

@jdisanti
Copy link
Contributor

jdisanti commented Nov 29, 2023

I think the HTTPS_NATIVE_ROOTS initialization is a red herring here, and that smithy-lang/smithy-rs#3269 should solve the root problem of initializing several clients costing 2-3 milliseconds each.

We should avoid loading HTTPS_NATIVE_ROOTS at all in the case where they're not used (when a custom client that doesn't use them is configured, for example), but that is a separate issue.

@jdisanti jdisanti added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Dec 1, 2023
@jdisanti
Copy link
Contributor

jdisanti commented Dec 1, 2023

This fix for this has been merged and will go out in an upcoming release (but not necessarily the next one).

@monken
Copy link
Author

monken commented Dec 1, 2023

Thank you!

@jdisanti
Copy link
Contributor

The fix for this went out in the December 14th release.

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.
Projects
Status: Done
Development

No branches or pull requests

3 participants