Repository: denoland/deno
Pull request: perf(install): keep parsed npm package info in memory cache

Fixes #28517.

The npm package info gets requested a bunch of times by deno_npm. Before this PR, we were loading it from the FS and parsing it each and every time. With a lot of dependencies (and large `registry.json` files), this can lead to massive blowups in install times.

From the repro in #28517

before this PR:

```
        Command being timed: "deno i"
        User time (seconds): 538.54
        System time (seconds): 56.49
        Percent of CPU this job got: 198%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 4:59.45
        Maximum resident set size (kbytes): 378976
```

this PR:
```
        Command being timed: "deno-this-pr i"
        User time (seconds): 1.29
        System time (seconds): 1.56
        Percent of CPU this job got: 68%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.16
        Maximum resident set size (kbytes): 500864
```

So roughly an improvement from 339s to 4s. You can see that the max RSS does increase a decent amount, which is the main downside. However, this in memory cache is cleared once we're done caching npm packages, and IMO the performance tradeoff is well worth it.

This also has a very noticable, though less drastic, effect on fresh installs (no deno.lock) for smaller projects. Here's a clean nextJS template project:

```
❯ hyperfine --warmup 5 --prepare "rm -rf node_modules deno.lock" "deno i" "deno-this-pr i"
Benchmark 1: deno
  Time (mean ± σ):     765.0 ms ±  10.1 ms    [User: 622.3 ms, System: 216.4 ms]
  Range (min … max):   749.0 ms … 783.6 ms    10 runs

Benchmark 2: deno-this-pr
  Time (mean ± σ):     357.2 ms ±   9.4 ms    [User: 193.2 ms, System: 198.2 ms]
  Range (min … max):   346.4 ms … 374.1 ms    10 runs

Summary
  deno-this-pr ran
    2.14 ± 0.06 times faster than deno
```


Changed files:
filename: resolvers/npm_cache/registry_info.rs
patch: @@ -48,7 +48,7 @@ enum MemoryCacheItem {
   /// The item has loaded in the past and was stored in the file system cache.
   /// There is no reason to request this package from the npm registry again
   /// for the duration of execution.
-  FsCached,
+  FsCached(Arc<NpmPackageInfo>),
   /// An item is memory cached when it fails saving to the file system cache
   /// or the package does not exist.
   MemoryCached(Result<Option<Arc<NpmPackageInfo>>, Arc<JsErrorBox>>),
@@ -64,6 +64,17 @@ impl MemoryCache {
   #[inline(always)]
   pub fn clear(&mut self) {
     self.clear_id += 1;
+
+    // if the item couldn't be saved to the fs cache, then we want to continue to hold it in memory
+    // to avoid re-downloading it from the registry
+    self
+      .items
+      .retain(|_, item| matches!(item, MemoryCacheItem::MemoryCached(Ok(_))));
+  }
+
+  #[inline(always)]
+  pub fn clear_all(&mut self) {
+    self.clear_id += 1;
     self.items.clear();
   }
 
@@ -96,18 +107,6 @@ impl MemoryCache {
   }
 }
 
-#[derive(Debug, thiserror::Error, deno_error::JsError)]
-#[class(generic)]
-pub enum LoadFileCachedPackageInfoError {
-  #[error("Previously saved '{name}' from the npm cache, but now it fails to load: {err}")]
-  LoadPackageInfo {
-    err: serde_json::Error,
-    name: String,
-  },
-  #[error("The package '{0}' previously saved its registry information to the file system cache, but that file no longer exists.")]
-  FileMissing(String),
-}
-
 #[derive(Debug, thiserror::Error, deno_error::JsError)]
 #[class(inherit)]
 #[error("Failed loading {url} for package \"{name}\"")]
@@ -120,15 +119,9 @@ pub struct LoadPackageInfoError {
 }
 
 #[derive(Debug, thiserror::Error, deno_error::JsError)]
-pub enum LoadPackageInfoInnerError {
-  #[class(inherit)]
-  #[error("{0}")]
-  LoadFileCachedPackageInfo(LoadFileCachedPackageInfoError),
-  #[class(inherit)]
-  #[error("{0}")]
-  Other(Arc<JsErrorBox>),
-}
-
+#[class(inherit)]
+#[error("{0}")]
+pub struct LoadPackageInfoInnerError(pub Arc<JsErrorBox>);
 // todo(#27198): refactor to store this only in the http cache
 
 /// Downloads packuments from the npm registry.
@@ -205,7 +198,7 @@ impl<
       return false;
     }
     if self.force_reload_flag.raise() {
-      self.clear_memory_cache();
+      self.memory_cache.lock().clear_all();
       true
     } else {
       false
@@ -268,16 +261,9 @@ impl<
     };
 
     match cache_item {
-      MemoryCacheItem::FsCached => {
-        // this struct previously loaded from the registry, so we can load it from the file system cache
-        self
-          .load_file_cached_package_info(name)
-          .await
-          .map(|info| Some(Arc::new(info)))
-          .map_err(LoadPackageInfoInnerError::LoadFileCachedPackageInfo)
-      }
+      MemoryCacheItem::FsCached(info) => Ok(Some(info)),
       MemoryCacheItem::MemoryCached(maybe_info) => {
-        maybe_info.clone().map_err(LoadPackageInfoInnerError::Other)
+        maybe_info.clone().map_err(LoadPackageInfoInnerError)
       }
       MemoryCacheItem::Pending(value_creator) => {
         match value_creator.get().await {
@@ -287,7 +273,7 @@ impl<
             self.memory_cache.lock().try_insert(
               clear_id,
               name,
-              MemoryCacheItem::FsCached,
+              MemoryCacheItem::FsCached(info.clone()),
             );
             Ok(Some(info))
           }
@@ -315,38 +301,13 @@ impl<
               name,
               MemoryCacheItem::MemoryCached(Err(err)),
             );
-            Err(LoadPackageInfoInnerError::Other(return_err))
+            Err(LoadPackageInfoInnerError(return_err))
           }
         }
       }
     }
   }
 
-  async fn load_file_cached_package_info(
-    &self,
-    name: &str,
-  ) -> Result<NpmPackageInfo, LoadFileCachedPackageInfoError> {
-    // this scenario failing should be exceptionally rare so let's
-    // deal with improving it only when anyone runs into an issue
-    let maybe_package_info = deno_unsync::spawn_blocking({
-      let cache = self.cache.clone();
-      let name = name.to_string();
-      move || cache.load_package_info(&name)
-    })
-    .await
-    .unwrap()
-    .map_err(|err| LoadFileCachedPackageInfoError::LoadPackageInfo {
-      err,
-      name: name.to_string(),
-    })?;
-    match maybe_package_info {
-      Some(package_info) => Ok(package_info),
-      None => Err(LoadFileCachedPackageInfoError::FileMissing(
-        name.to_string(),
-      )),
-    }
-  }
-
   fn create_load_future(self: &Arc<Self>, name: &str) -> LoadFuture {
     let downloader = self.clone();
     let package_url = get_package_url(&self.npmrc, name);