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

Investigate overhead in initializing HTTP client #2346

Closed
charliermarsh opened this issue Mar 10, 2024 · 5 comments · Fixed by #2362
Closed

Investigate overhead in initializing HTTP client #2346

charliermarsh opened this issue Mar 10, 2024 · 5 comments · Fixed by #2362
Assignees
Labels
macos Specific to the macOS platform performance Potential performance improvement

Comments

@charliermarsh
Copy link
Member

If I add debug logging before and after:

// Initialize the base client.
let client = self.client.unwrap_or_else(|| {
    // Disallow any connections.
    let client_core = ClientBuilder::new()
        .user_agent(user_agent_string)
        .pool_max_idle_per_host(20)
        .timeout(std::time::Duration::from_secs(timeout));

    client_core.build().expect("Failed to build HTTP client.")
});

...in the registry client, it says it takes >120ms to initialize the client. That's a ton! In many cases, it's more expensive than the resolution itself.

Did this regress at some point? My best guess would be that we regressed by reading the system certificates, but we should look into it.

@charliermarsh charliermarsh added the performance Potential performance improvement label Mar 10, 2024
@charliermarsh
Copy link
Member Author

Yes, confirmed, dropping rustls-tls-native-roots removes this overhead.

@charliermarsh
Copy link
Member Author

I would be curious if this reproduces on Linux at all, or if it's macOS-only.

@charliermarsh
Copy link
Member Author

Either way, I’m going to make the client initialization lazy.

charliermarsh added a commit that referenced this issue Mar 11, 2024
## Summary

We now initialize an HTTP client in advance for remote requirements
files. It turns out this adds a significant overhead, even for
operations like auditing the environment (at least on macOS).

This PR makes initialization lazy. After a lot of evaluation, I took the
easiest route, which is: we just pass in `Connectivity`, and then use
the default HTTP client. So we won't respect netrc files and anything
else that we get from our registry client. If we want to keep using the
registry client, we _can_, it's just way more ceremony to pass down a
closure.

See: #2346.

## Test Plan

- Verified that `cargo run pip compile
https://raw.githubusercontent.com/ansible/ansible/f1ded0f41759235eb15a7d13dbc3c95dce5d5acd/requirements.txt`
completed without error.
- Verified that `cargo run pip compile
https://raw.githubusercontent.com/ansible/ansible/f1ded0f41759235eb15a7d13dbc3c95dce5d5acd/requirements.txt
--offline` failed with an error.
- Verified that `./target/release/uv pip install requests` completed in
0-2ms, rather than hundreds.
@konstin
Copy link
Member

konstin commented Mar 11, 2024

Is that mac specific or do i need to set an environment variable? I tried to reproduce but client building is 4ms on my ubuntu machine.

@charliermarsh
Copy link
Member Author

I believe it’s specific to macOS.

@konstin konstin added the macos Specific to the macOS platform label Mar 11, 2024
@charliermarsh charliermarsh self-assigned this Mar 11, 2024
charliermarsh added a commit that referenced this issue Mar 12, 2024
## Summary

It turns out that on macOS, reading the native certificates can add
hundreds of milliseconds to client initialization. This PR makes
`--native-tls` a command-line flag, to toggle (at runtime) the choice of
the `webpki` roots or the native system roots.

You can't accomplish this kind of configuration with the `reqwest`
builder API, so instead, I pulled out the heart of that logic from the
crate
(https://github.com/seanmonstar/reqwest/blob/e3192638518d577759dd89da489175b8f992b12f/src/async_impl/client.rs#L498),
and modified it to allow toggling a choice of root.

Note that there's an open PR for this in reqwest
(seanmonstar/reqwest#1848), along with an issue
(seanmonstar/reqwest#1843), which I may ping,
but it's been around for a while and I believe reqwest is focused on its
next major release.

Closes #2346.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos Specific to the macOS platform performance Potential performance improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants