Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: add http caching to reqwest#75

Open
anonrig wants to merge 1 commit into
mainfrom
http-cache
Open

feat: add http caching to reqwest#75
anonrig wants to merge 1 commit into
mainfrom
http-cache

Conversation

@anonrig
Copy link
Copy Markdown
Member

@anonrig anonrig commented Aug 11, 2023

before

Benchmark 1: pacquet
  Time (mean ± σ):     799.0 ms ±  17.2 ms    [User: 141.8 ms, System: 818.4 ms]
  Range (min … max):   777.5 ms … 827.8 ms    10 runs

after

Benchmark 1: pacquet
  Time (mean ± σ):     656.5 ms ±  26.4 ms    [User: 152.1 ms, System: 811.8 ms]
  Range (min … max):   625.6 ms … 709.3 ms    10 runs

@anonrig anonrig requested a review from KSXGitHub August 11, 2023 15:31
KSXGitHub
KSXGitHub reviewed Aug 11, 2023
View reviewed changes
Comment thread crates/cli/Cargo.toml Outdated
Comment on lines +26 to +37
clap = { workspace = true }
futures-util = { workspace = true }
rayon = { workspace = true }
reflink-copy = { workspace = true }
reqwest = { workspace = true }
node-semver = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
tokio = { workspace = true }
clap = { workspace = true }
http-cache-reqwest = { workspace = true }
futures-util = { workspace = true }
moka = { workspace = true }
rayon = { workspace = true }
reflink-copy = { workspace = true }
reqwest = { workspace = true }
reqwest-middleware = { workspace = true }
node-semver = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
tokio = { workspace = true }
Copy link
Copy Markdown
Contributor

@KSXGitHub KSXGitHub Aug 11, 2023

Choose a reason for hiding this comment

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

See? This is the huge diff that I talked about.

@KSXGitHub
Copy link
Copy Markdown
Contributor

KSXGitHub commented Aug 11, 2023

Anyway, I will review this later. Maybe tomorrow, or maybe next week.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 11, 2023 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.39%. Comparing base (40f1291) to head (406dfc8).
⚠️ Report is 95 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main      #75   +/-   ##
=======================================
  Coverage   85.39%   85.39%           
=======================================
  Files          24       24           
  Lines        1239     1253   +14     
=======================================
+ Hits         1058     1070   +12     
- Misses        181      183    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 11, 2023

Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.05     10.2±0.60ms   426.6 KB/sec    1.00      9.7±0.53ms   447.2 KB/sec

@KSXGitHub
Copy link
Copy Markdown
Contributor

KSXGitHub commented Aug 11, 2023

I will make a prediction: This will likely be rendered irrelevant once a proper hashmap-based caching mechanism is implemented.

KSXGitHub
KSXGitHub reviewed Aug 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@KSXGitHub KSXGitHub left a comment

Choose a reason for hiding this comment

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

You should fix the cargo deny.

@anonrig
Copy link
Copy Markdown
Member Author

anonrig commented Aug 14, 2023

You should fix the cargo deny.

I think we shouldn't follow this pattern, and start working on the hashmap-based caching mechanism.

@KSXGitHub
Copy link
Copy Markdown
Contributor

KSXGitHub commented Aug 24, 2023

@anonrig Since we have merged hashmap based caching mechanism and full parallelization, you can now do a benchmark (in pacquet install) to see if this PR could bring performance improvement.

@await-ovo
Copy link
Copy Markdown
Member

await-ovo commented Nov 24, 2023

It looks like we've just added mem-cache to the tarball download, should we add a cache for fetching the package from the registry as well?

Whether we implement mem-cache or not, perhaps http_caching stored on disk still has some value?

@KSXGitHub
Copy link
Copy Markdown
Contributor

KSXGitHub commented Nov 24, 2023

@await-ovo Currently, all network requests in pacquet is performed via a single wrapper: pacquet-network. Cache may be added there.

@zkochan zkochan self-requested a review as a code owner May 12, 2026 20:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zkochan zkochan Awaiting requested review from zkochan zkochan is a code owner

1 more reviewer

@KSXGitHub KSXGitHub KSXGitHub left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anonrig @KSXGitHub @await-ovo