feat: hashmap based caching #82

KSXGitHub · 2023-08-15T15:24:47Z

Before this PR, pacquet install was installing packages by "generations": All direct dependencies are installed first, then dependencies of direct dependencies, then dependencies of dependencies of dependencies, and so on. This was not optimal, but it was done to avoid race condition where some packages would be fetched twice should they be invoked in the window of time where its duplicated has begun fetching but not yet saved to local disk.

This PR aims to change this by using an in-memory hashmap cache to keep track of all fetched packages, then re-enable full parallelization.

NOTE: This PR only affects pacquet install, pacquet add still uses the old algorithm.

NOTE: New tests with multiple packages for pacquet install is required, but I am too tired right now.

lossy shouldn't be used here.

crates/cli/src/package_cache.rs

anonrig · 2023-08-15T15:30:07Z

crates/cli/src/package.rs

+        .join(&package_version.name);
+
+    if let Some(mut receiver) = package_cache.get_mut(&saved_path) {
+        // TODO: is this loop necessary?


If we use a Mutex<Receiver<PackageState>> we don't have to have a loop here. Similar to: https://medium.com/@polyglot_factotum/rust-concurrency-patterns-condvars-and-locks-e278f18db74f

This is not what my question was referring to. I am wonder whether I should remove the whole code block inside loop (leaving only return Ok(())) or keep it. This would only be clear after further work. Hence the TODO.

If the package is InProgress we need to wait, since after this function, we call package import functions to link the store files with node_modules.

anonrig · 2023-08-15T15:31:30Z

crates/tarball/src/lib.rs

-        Ok(cas_files)
-    })
-    .await?
+    verify_checksum(&response, package_integrity)?;


Why did we remove tokio spawn in here? The tarball extraction is a CPU bounded problem, and solving it in a different thread helps the cause?

We were going to .await on the handle right afterward. So even if we spawn another thread, the current thread will be waiting for the spawned thread to complete. It will just like not spawning any thread at all.

crates/tarball/src/lib.rs

crates/cli/src/package.rs

codecov · 2023-08-15T15:37:42Z

Codecov Report

Patch coverage: 78.98% and project coverage change: -1.91% ⚠️

Comparison is base (022ca6e) 85.82% compared to head (c386708) 83.92%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   85.82%   83.92%   -1.91%     
==========================================
  Files          24       24              
  Lines        1270     1306      +36     
==========================================
+ Hits         1090     1096       +6     
- Misses        180      210      +30

Files Changed	Coverage Δ
crates/cli/src/lib.rs	`43.42% <0.00%> (-0.58%)`	⬇️
crates/npmrc/src/lib.rs	`94.82% <ø> (ø)`
crates/registry/src/lib.rs	`0.00% <0.00%> (ø)`
crates/tarball/src/lib.rs	`79.54% <48.48%> (-19.40%)`	⬇️
crates/cli/src/package_import.rs	`72.72% <60.00%> (-13.39%)`	⬇️
crates/cli/src/package_manager.rs	`77.77% <80.00%> (-7.94%)`	⬇️
crates/registry/src/package.rs	`89.61% <81.81%> (+0.13%)`	⬆️
crates/cli/src/commands/install.rs	`96.03% <95.00%> (-1.51%)`	⬇️
crates/cafs/src/lib.rs	`98.03% <100.00%> (ø)`
crates/cli/src/commands/add.rs	`94.77% <100.00%> (+0.32%)`	⬆️
... and 3 more

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

github-actions · 2023-08-15T15:39:00Z

Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      9.7±0.47ms   445.8 KB/sec    1.02      9.9±0.58ms   438.2 KB/sec

zkochan · 2023-08-21T22:58:10Z

Could you add a description what this PR is about?

KSXGitHub · 2023-08-22T04:12:32Z

Could you add a description what this PR is about?

done

zkochan · 2023-08-22T22:10:29Z

crates/cli/src/package.rs

+        .join("node_modules")
+        .join(&package_version.name);
+
+    // TODO: skip when it already exists in store?


Do you mean the virtual store at node_modules/.pnpm?

This is global store.

zkochan · 2023-08-22T22:13:50Z

crates/cli/src/package.rs


+    let saved_path = config


I think import_destination or import_dest would be a better name for the variable.

However, the argument name in the function is save_path, so better name the same, not "saveD_path"

zkochan · 2023-08-22T23:32:55Z

crates/registry/src/package_distribution.rs

@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
 #[derive(Serialize, Deserialize, Debug, Default, Clone, Eq)]
 #[serde(rename_all = "camelCase")]
 pub struct PackageDistribution {
-    pub integrity: String,
+    pub integrity: String, // TODO: why fetching typescript cause error here?


Will we merge the PR with TODO items? It is probably best to open issues instead of adding TODO comments.

I personally prefer code todos as well as issues. It makes it easier to navigate through the codebase.

zkochan · 2023-08-22T23:35:49Z

crates/tarball/src/lib.rs

+            }
+        }
+        drop(cache_lock);
+        sleep(Duration::from_millis(100)).await; // TODO: millis can be any small number, even 0, further testing is required to find the ideal number


Is it OK to use sleep for this? Looks like a workaround.

Why do we even need sleep in here? We need more comments on the code in general.

anonrig · 2023-08-23T00:05:05Z

crates/cli/src/commands/install.rs

+                    &self.http_client,
+                    name,
+                    version,
+                    &self.config.modules_dir,


We are already passing self.config to this function. We dont need this do we?

anonrig · 2023-08-23T00:05:37Z

crates/cli/src/package.rs


+    let saved_path = config


anonrig · 2023-08-23T00:05:55Z

crates/cli/src/package.rs

+        .join("node_modules")
+        .join(&package_version.name);
+
+    // TODO: skip when it already exists in store?


This is global store.

anonrig · 2023-08-23T00:08:24Z

crates/registry/src/package_distribution.rs

@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
 #[derive(Serialize, Deserialize, Debug, Default, Clone, Eq)]
 #[serde(rename_all = "camelCase")]
 pub struct PackageDistribution {
-    pub integrity: String,
+    pub integrity: String, // TODO: why fetching typescript cause error here?


I personally prefer code todos as well as issues. It makes it easier to navigate through the codebase.

anonrig · 2023-08-23T00:09:01Z

crates/registry/src/package_version.rs

@@ -30,13 +29,18 @@ impl PackageVersion {
        http_client: &reqwest::Client,
        registry: &str,
    ) -> Result<Self, RegistryError> {
+        let url = || format!("{0}{name}/{version}", &registry); // TODO: use reqwest url type
+        let network_error = |error| NetworkError { error, url: url() };
+
        http_client
            .get(format!("{0}{name}/{version}", &registry))


This line needs to be updated to use local variable url.

anonrig · 2023-08-23T00:09:16Z

crates/registry/src/package_version.rs

@@ -30,13 +29,18 @@ impl PackageVersion {
        http_client: &reqwest::Client,
        registry: &str,
    ) -> Result<Self, RegistryError> {
+        let url = || format!("{0}{name}/{version}", &registry); // TODO: use reqwest url type


Why do we need this todo?

The current type of registry is &str, and it only works correctly when it ends with a /. The Url type is more efficient and less error-prone.

anonrig · 2023-08-23T00:10:51Z

crates/tarball/src/lib.rs

+            }
+        }
+        drop(cache_lock);
+        sleep(Duration::from_millis(100)).await; // TODO: millis can be any small number, even 0, further testing is required to find the ideal number


Why do we even need sleep in here? We need more comments on the code in general.

`sleep` is just `yield_now` with extra steps

anonrig · 2023-08-23T18:21:31Z

crates/cli/Cargo.toml

-serde        = { workspace = true }
-serde_json   = { workspace = true }
-tokio        = { workspace = true }
+async-recursion = { workspace = true }


Are we using async-recursion? I couldn't find any reference.

pacquet/crates/cli/src/commands/install.rs

Line 48 in 9c94551

#[async_recursion]

KSXGitHub added 8 commits August 15, 2023 12:25

refactor: naming convention

7f32245

refactor: remove useless task spawn

785e878

feat: panic on invalid UTF-8

54b7cc1

lossy shouldn't be used here.

refactor: remove unnecessary copy and allocation

f35fdff

docs: add a TODO

fe0583d

refactor: remove node_modules_path

3b600d9

feat: hashmap caching communication

5301577

feat: log cache hit

cbfe18c

KSXGitHub requested review from zkochan and anonrig August 15, 2023 15:24

chore: merge from main

e83c4c0

anonrig reviewed Aug 15, 2023

View reviewed changes

crates/cli/src/package.rs Outdated Show resolved Hide resolved

KSXGitHub added 15 commits August 15, 2023 22:39

feat: use tracing library

ed708b1

feat: propagate archive error

0fe3e95

docs: change the TODO

f7b32cd

refactor: rename cache state

7bcaf06

feat: remove unused function

4d1b724

docs: ask a question

2103470

feat: import on cache hit

85dc278

feat: log saved_path

ca7de84

feat: log cache miss

d06fd00

feat: change tracing level

a23fc50

style: consistent string substitution style

8c4c276

docs: add a TODO

21a2f89

docs: add a TODO

6bd33e9

docs: remove poor comment

10b8be0

docs: add a TODO

242c189

feat: log beginning and end of sets and subsets

479e7e3

KSXGitHub added 2 commits August 22, 2023 14:59

feat: drop lock and sleep

fe32b99

docs: millis

9b24ea1

KSXGitHub marked this pull request as ready for review August 22, 2023 08:11

KSXGitHub requested a review from anonrig August 22, 2023 08:11

chore: remove an unused dependency

80346b6

zkochan reviewed Aug 22, 2023

View reviewed changes

zkochan approved these changes Aug 22, 2023

View reviewed changes

anonrig reviewed Aug 23, 2023

View reviewed changes

KSXGitHub added 9 commits August 23, 2023 10:34

refactor: change var name

3c380a5

refactor: only compute save_path when necessary

3c3f4b8

docs: remove a TODO

b921350

style: fix string interpolation style

fc3e7d0

docs: move a TODO

104fbcd

docs: clarify

baa9b82

feat: change log message

c736d86

fix: log message grammar

d5eb906

refactor: remove box

90eb4d5

KSXGitHub requested review from anonrig and zkochan August 23, 2023 03:59

KSXGitHub added 4 commits August 23, 2023 11:16

docs: remove the question

42422bd

feat: reduce panic by using OsString

b76f3a4

feat: replace sleep with yield_now

663dd8d

`sleep` is just `yield_now` with extra steps

refactor: forgot to reuse url

c386708

anonrig approved these changes Aug 23, 2023

View reviewed changes

zkochan approved these changes Aug 24, 2023

View reviewed changes

zkochan merged commit 9c94551 into main Aug 24, 2023
15 of 17 checks passed

zkochan deleted the hashmap-based-caching branch August 24, 2023 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hashmap based caching #82

feat: hashmap based caching #82

KSXGitHub commented Aug 15, 2023 •

edited

anonrig Aug 15, 2023

KSXGitHub Aug 15, 2023

anonrig Aug 15, 2023

anonrig Aug 15, 2023

KSXGitHub Aug 15, 2023 •

edited

codecov bot commented Aug 15, 2023 •

edited

github-actions bot commented Aug 15, 2023 •

edited

zkochan commented Aug 21, 2023

KSXGitHub commented Aug 22, 2023

zkochan Aug 22, 2023

anonrig Aug 23, 2023

zkochan Aug 22, 2023

anonrig Aug 23, 2023

zkochan Aug 22, 2023

anonrig Aug 23, 2023

KSXGitHub Aug 23, 2023

zkochan Aug 22, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

KSXGitHub Aug 23, 2023

anonrig Aug 23, 2023

anonrig Aug 23, 2023

KSXGitHub Aug 24, 2023

feat: hashmap based caching #82

feat: hashmap based caching #82

Conversation

KSXGitHub commented Aug 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KSXGitHub Aug 15, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2023 • edited

Codecov Report

github-actions bot commented Aug 15, 2023 • edited

Benchmark Results

Linux

zkochan commented Aug 21, 2023

KSXGitHub commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KSXGitHub commented Aug 15, 2023 •

edited

KSXGitHub Aug 15, 2023 •

edited

codecov bot commented Aug 15, 2023 •

edited

github-actions bot commented Aug 15, 2023 •

edited