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

Only perform fetches of credentials for a realm once #3237

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Apr 24, 2024

Closes #3205

Tested with

RUST_LOG=uv=trace cargo run -- pip install -r scripts/requirements/trio.in --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess -vv --reinstall 2>&1 | grep keyring

On main you can see a dozen keyring attempts at once. Here, the other requests wait for the first attempt and only a single keyring call is performed.

@zanieb zanieb added the performance Potential performance improvement label Apr 24, 2024
Comment on lines 12 to +15
/// A cache per realm and username
realms: Mutex<HashMap<(Realm, Username), Arc<Credentials>>>,
/// A cache tracking the result of fetches from external services
pub(crate) fetches: OnceMap<(Realm, Username), Option<Arc<Credentials>>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

This duplication hints that we should refactor the realm cache but I don't want to make further changes here.

Copy link
Member

Choose a reason for hiding this comment

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

pub(crate) seems slightly off but perhaps not worth exposing these as methods.

# Conflicts:
#	crates/uv-auth/Cargo.toml
@zanieb zanieb marked this pull request as ready for review April 24, 2024 14:09
@zanieb
Copy link
Member Author

zanieb commented Apr 24, 2024

Going to do some sort of light benchmarking.

@zanieb
Copy link
Member Author

zanieb commented Apr 24, 2024

Looks helpful for large requirements.

Note for all of these I seeded a clean environment first to ensure there wasn't an extra cost for the first invocation.

❯ hyperfine -n branch -n main -- "./branch pip install -r ../prefect/requirements.txt --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q" "./main pip install -r ../prefect/requirements.txt --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q"
Benchmark 1: branch
  Time (mean ± σ):     10.714 s ±  3.331 s    [User: 1.595 s, System: 2.350 s]
  Range (min … max):    7.503 s … 15.264 s    10 runs
 
Benchmark 2: main
  Time (mean ± σ):     16.423 s ±  2.307 s    [User: 26.098 s, System: 8.084 s]
  Range (min … max):   13.985 s … 19.846 s    10 runs
 
Summary
  branch ran
    1.53 ± 0.52 times faster than main
    
❯ hyperfine -n branch -n main -- "./branch pip install -r scripts/requirements/trio.in --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q" "./main pip install -r scripts/requirements/trio.in --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q"
Benchmark 1: branch
  Time (mean ± σ):     12.766 s ±  2.964 s    [User: 1.449 s, System: 1.536 s]
  Range (min … max):    8.814 s … 16.260 s    10 runs
 
Benchmark 2: main
  Time (mean ± σ):     13.331 s ±  3.146 s    [User: 8.309 s, System: 3.164 s]
  Range (min … max):    8.575 s … 17.044 s    10 runs
 
Summary
  branch ran
    1.04 ± 0.35 times faster than main

❯  hyperfine -n branch -n main -- "./branch pip install -r scripts/requirements/compiled/jupyter.txt --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q" "./main pip install -r scripts/requirements/compiled/jupyter.txt --index-url https://oauth2accesstoken@us-central1-python.pkg.dev/zb-test-project-421213/pypyi/simple/ --no-cache --keyring-provider subprocess --reinstall -q"
Benchmark 1: branch
  Time (mean ± σ):     13.692 s ±  3.222 s    [User: 2.062 s, System: 2.956 s]
  Range (min … max):    8.369 s … 17.495 s    10 runs
 
Benchmark 2: main
  Time (mean ± σ):     15.738 s ±  1.982 s    [User: 27.028 s, System: 8.893 s]
  Range (min … max):   13.242 s … 19.283 s    10 runs
 
Summary
  branch ran
    1.15 ± 0.31 times faster than main

@zanieb zanieb merged commit e92b38c into main Apr 24, 2024
40 checks passed
@zanieb zanieb deleted the zb/auth-fetch-once branch April 24, 2024 14:53
zanieb added a commit that referenced this pull request Apr 24, 2024
This is handled by `CredentialsCache.fetches` instead since #3237 

Moves the test demonstrating the flaw in the cache to the middleware
level.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huge slowdown when using keyring(google artifact registry) in 0.1.34/35 and auth problem in 0.1.36
2 participants