refactor: use a single Mutex in ProcState for module graph (#12489)

This commit factors out 4 different fields from "ProcState", that are behind
"Arc<Mutex<>>" into a single struct behind a single mutex.

cli/main.rs

@@ -548,7 +548,7 @@ async fn cache_command(
       Permissions::allow_all(),
     )
     .await?;
-    if let Some(graph_error) = ps.maybe_graph_error.lock().take() {
+    if let Some(graph_error) = ps.take_graph_error() {
       return Err(graph_error.into());
     }
   }

cli/proc_state.rs

@@ -52,31 +52,35 @@ use std::sync::Arc;
 #[derive(Clone)]
 pub struct ProcState(Arc<Inner>);
 
+#[derive(Default)]
+struct GraphData {
+  modules: HashMap<ModuleSpecifier, Result<ModuleSource, AnyError>>,
+  // because the graph detects resolution issues early, but is build and dropped
+  // during the `prepare_module_load` method, we need to extract out the module
+  // resolution map so that those errors can be surfaced at the appropriate time
+  resolution_map:
+    HashMap<ModuleSpecifier, HashMap<String, deno_graph::Resolved>>,
+  // in some cases we want to provide the span where the resolution error
+  // occurred but need to surface it on load, but on load we don't know who the
+  // referrer and span was, so we need to cache those
+  resolved_map: HashMap<ModuleSpecifier, deno_graph::Span>,
+  // deno_graph detects all sorts of issues at build time (prepare_module_load)
+  // but if they are errors at that stage, the don't cause the correct behaviors
+  // so we cache the error and then surface it when appropriate (e.g. load)
+  maybe_graph_error: Option<deno_graph::ModuleGraphError>,
+}
+
 pub struct Inner {
   /// Flags parsed from `argv` contents.
   pub flags: flags::Flags,
   pub dir: deno_dir::DenoDir,
   pub coverage_dir: Option<String>,
   pub file_fetcher: FileFetcher,
-  modules: Arc<Mutex<HashMap<ModuleSpecifier, Result<ModuleSource, AnyError>>>>,
+  graph_data: Arc<Mutex<GraphData>>,
   pub lockfile: Option<Arc<Mutex<Lockfile>>>,
   pub maybe_config_file: Option<ConfigFile>,
   pub maybe_import_map: Option<ImportMap>,
   pub maybe_inspector_server: Option<Arc<InspectorServer>>,
-  // deno_graph detects all sorts of issues at build time (prepare_module_load)
-  // but if they are errors at that stage, the don't cause the correct behaviors
-  // so we cache the error and then surface it when appropriate (e.g. load)
-  pub(crate) maybe_graph_error:
-    Arc<Mutex<Option<deno_graph::ModuleGraphError>>>,
-  // because the graph detects resolution issues early, but is build and dropped
-  // during the `prepare_module_load` method, we need to extract out the module
-  // resolution map so that those errors can be surfaced at the appropriate time
-  resolution_map:
-    Arc<Mutex<HashMap<ModuleSpecifier, HashMap<String, deno_graph::Resolved>>>>,
-  // in some cases we want to provide the span where the resolution error
-  // occurred but need to surface it on load, but on load we don't know who the
-  // referrer and span was, so we need to cache those
-  resolved_map: Arc<Mutex<HashMap<ModuleSpecifier, deno_graph::Span>>>,
   pub root_cert_store: Option<RootCertStore>,
   pub blob_store: BlobStore,
   pub broadcast_channel: InMemoryBroadcastChannel,
@@ -233,14 +237,11 @@ impl ProcState {
       coverage_dir,
       flags,
       file_fetcher,
-      modules: Default::default(),
+      graph_data: Default::default(),
       lockfile,
       maybe_config_file,
       maybe_import_map,
       maybe_inspector_server,
-      maybe_graph_error: Default::default(),
-      resolution_map: Default::default(),
-      resolved_map: Default::default(),
       root_cert_store: Some(root_cert_store.clone()),
       blob_store,
       broadcast_channel,
@@ -249,6 +250,12 @@ impl ProcState {
     })))
   }
 
+  pub(crate) fn take_graph_error(
+    &self,
+  ) -> Option<deno_graph::ModuleGraphError> {
+    self.graph_data.lock().maybe_graph_error.take()
+  }
+
   /// Return any imports that should be brought into the scope of the module
   /// graph.
   fn get_maybe_imports(&self) -> Option<Vec<(ModuleSpecifier, Vec<String>)>> {
@@ -325,8 +332,8 @@ impl ProcState {
     // Determine any modules that have already been emitted this session and
     // should be skipped.
     let reload_exclusions: HashSet<ModuleSpecifier> = {
-      let modules = self.modules.lock();
+      let graph_data = self.graph_data.lock();
-      modules.keys().cloned().collect()
+      graph_data.modules.keys().cloned().collect()
     };
 
     let config_type = if self.flags.no_check {
@@ -409,26 +416,23 @@ impl ProcState {
       }
     }
 
+    {
+      let mut graph_data = self.graph_data.lock();
       // we iterate over the graph, looking for any modules that were emitted, or
       // should be loaded as their un-emitted source and add them to the in memory
       // cache of modules for loading by deno_core.
-    {
+      graph_data
-      let mut modules = self.modules.lock();
+        .modules
-      modules.extend(emit::to_module_sources(graph.as_ref(), &cache));
+        .extend(emit::to_module_sources(graph.as_ref(), &cache));
-    }
 
       // since we can't store the graph in proc state, because proc state needs to
       // be thread safe because of the need to provide source map resolution and
       // the graph needs to not be thread safe (due to wasmbind_gen constraints),
       // we have no choice but to extract out other meta data from the graph to
       // provide the correct loading behaviors for CLI
-    {
+      graph_data.resolution_map.extend(graph.resolution_map());
-      let mut resolution_map = self.resolution_map.lock();
+
-      resolution_map.extend(graph.resolution_map());
+      graph_data.maybe_graph_error = maybe_graph_error;
-    }
-    {
-      let mut self_maybe_graph_error = self.maybe_graph_error.lock();
-      *self_maybe_graph_error = maybe_graph_error;
     }
 
     // any updates to the lockfile should be updated now
@@ -445,17 +449,22 @@ impl ProcState {
     specifier: &str,
     referrer: &str,
   ) -> Result<ModuleSpecifier, AnyError> {
-    let resolution_map = self.resolution_map.lock();
+    if let Ok(s) = deno_core::resolve_url_or_path(referrer) {
-    if let Some((_, Some(map))) = deno_core::resolve_url_or_path(referrer)
+      let maybe_resolved = {
-      .ok()
+        let graph_data = self.graph_data.lock();
-      .map(|s| (s.clone(), resolution_map.get(&s)))
+        let resolved_specifier = graph_data
-    {
+          .resolution_map
-      if let Some(resolved) = map.get(specifier) {
+          .get(&s)
+          .and_then(|map| map.get(specifier));
+        resolved_specifier.cloned()
+      };
+
+      if let Some(resolved) = maybe_resolved {
         match resolved {
           Some(Ok((specifier, span))) => {
-            let mut resolved_map = self.resolved_map.lock();
+            let mut graph_data = self.graph_data.lock();
-            resolved_map.insert(specifier.clone(), span.clone());
+            graph_data.resolved_map.insert(specifier.clone(), span);
-            return Ok(specifier.clone());
+            return Ok(specifier);
           }
           Some(Err(err)) => {
             return Err(custom_error(
@@ -467,6 +476,7 @@ impl ProcState {
         }
       }
     }
+
     // FIXME(bartlomieju): hacky way to provide compatibility with repl
     let referrer = if referrer.is_empty() && self.flags.repl {
       deno_core::DUMMY_SPECIFIER
@@ -497,22 +507,46 @@ impl ProcState {
         .unwrap_or_else(|| "<none>".to_string()),
       is_dynamic
     );
-    let modules = self.modules.lock();
+
-    modules
+    {
+      let graph_data = self.graph_data.lock();
+      if let Some(module_result) = graph_data.modules.get(&specifier) {
+        if let Ok(module_source) = module_result {
+          return Ok(module_source.clone());
+        }
+      } else {
+        if maybe_referrer.is_some() && !is_dynamic {
+          if let Some(span) = graph_data.resolved_map.get(&specifier) {
+            return Err(custom_error(
+              "NotFound",
+              format!("Cannot load module \"{}\".\n    at {}", specifier, span),
+            ));
+          }
+        }
+        return Err(custom_error(
+          "NotFound",
+          format!("Cannot load module \"{}\".", specifier),
+        ));
+      }
+    }
+
+    // If we're this far it means that there was an error for this module load.
+    let mut graph_data = self.graph_data.lock();
+    let err = graph_data
+      .modules
       .get(&specifier)
-      .map(|r| match r {
+      .unwrap()
-        Ok(module_source) => Ok(module_source.clone()),
+      .as_ref()
-        Err(err) => {
+      .unwrap_err();
     // this is the "pending" error we will return
     let err = if let Some(error_class) = get_custom_error_class(err) {
       if error_class == "NotFound" && maybe_referrer.is_some() && !is_dynamic {
-              let resolved_map = self.resolved_map.lock();
         // in situations where we were to try to load a module that wasn't
         // emitted and we can't run the original source code (it isn't)
         // JavaScript, we will load a blank module instead.  This is
         // usually caused by people exporting type only exports and not
         // type checking.
-              if let Some(span) = resolved_map.get(&specifier) {
+        if let Some(span) = graph_data.resolved_map.get(&specifier) {
           log::warn!("{}: Cannot load module \"{}\".\n    at {}\n  If the source module contains only types, use `import type` and `export type` to import it instead.", colors::yellow("warning"), specifier, span);
           return Ok(ModuleSource {
             code: "".to_string(),
@@ -527,13 +561,14 @@ impl ProcState {
     };
     // if there is a pending graph error though we haven't returned, we
     // will return that one
-          let mut maybe_graph_error = self.maybe_graph_error.lock();
+    if let Some(graph_error) = graph_data.maybe_graph_error.take() {
-          if let Some(graph_error) = maybe_graph_error.take() {
       log::debug!("returning cached graph error");
-            let resolved_map = self.resolved_map.lock();
+      if let Some(span) = graph_data.resolved_map.get(&specifier) {
-            if let Some(span) = resolved_map.get(&specifier) {
         if !span.specifier.as_str().contains("$deno") {
-                return Err(custom_error(get_module_graph_error_class(&graph_error), format!("{}\n    at {}", graph_error, span)));
+          return Err(custom_error(
+            get_module_graph_error_class(&graph_error),
+            format!("{}\n    at {}", graph_error, span),
+          ));
         }
       }
       Err(graph_error.into())
@@ -541,17 +576,6 @@ impl ProcState {
       Err(err)
     }
   }
-      })
-      .unwrap_or_else(|| {
-        if maybe_referrer.is_some() && !is_dynamic {
-          let resolved_map = self.resolved_map.lock();
-          if let Some(span) = resolved_map.get(&specifier) {
-            return Err(custom_error("NotFound", format!("Cannot load module \"{}\".\n    at {}", specifier, span)));
-          }
-        }
-        Err(custom_error("NotFound", format!("Cannot load module \"{}\".", specifier)))
-      })
-  }
 
   // TODO(@kitsonk) this should be refactored to get it from the module graph
   fn get_emit(&self, url: &Url) -> Option<(Vec<u8>, Option<Vec<u8>>)> {