refactor(core): don't depend on get_identity_hash for modules (#8354)

This commit refactors "deno_core::Modules" structure to not depend on
"get_identity_hash" function to identify modules, but instead use default
hash implementation.

core/bindings.rs

@@ -264,10 +264,11 @@ pub extern "C" fn host_initialize_import_meta_object_callback(
   let state_rc = JsRuntime::state(scope);
   let state = state_rc.borrow();
 
-  let id = module.get_identity_hash();
-  assert_ne!(id, 0);
-
-  let info = state.modules.get_info(id).expect("Module not found");
+  let module_global = v8::Global::new(scope, module);
+  let info = state
+    .modules
+    .get_info(&module_global)
+    .expect("Module not found");
 
   let url_key = v8::String::new(scope, "url").unwrap();
   let url_val = v8::String::new(scope, &info.name).unwrap();
@@ -713,6 +714,7 @@ fn shared_getter(
   rv.set(shared_ab.into())
 }
 
+// Called by V8 during `Isolate::mod_instantiate`.
 pub fn module_resolve_callback<'s>(
   context: v8::Local<'s, v8::Context>,
   specifier: v8::Local<'s, v8::String>,
@@ -721,39 +723,38 @@ pub fn module_resolve_callback<'s>(
   let scope = &mut unsafe { v8::CallbackScope::new(context) };
 
   let state_rc = JsRuntime::state(scope);
-  let mut state = state_rc.borrow_mut();
+  let state = state_rc.borrow();
 
-  let referrer_id = referrer.get_identity_hash();
-  let referrer_name = state
+  let referrer_global = v8::Global::new(scope, referrer);
+  let referrer_info = state
     .modules
-    .get_info(referrer_id)
-    .expect("ModuleInfo not found")
-    .name
-    .to_string();
-  let len_ = referrer.get_module_requests_length();
+    .get_info(&referrer_global)
+    .expect("ModuleInfo not found");
+  let referrer_name = referrer_info.name.to_string();
 
   let specifier_str = specifier.to_rust_string_lossy(scope);
 
-  for i in 0..len_ {
-    let req = referrer.get_module_request(i);
-    let req_str = req.to_rust_string_lossy(scope);
+  let resolved_specifier = state
+    .loader
+    .resolve(
+      state.op_state.clone(),
+      &specifier_str,
+      &referrer_name,
+      false,
+    )
+    .expect("Module should have been already resolved");
 
-    if req_str == specifier_str {
-      let id = state.module_resolve_cb(&req_str, referrer_id);
-      match state.modules.get_info(id) {
-        Some(info) => return Some(v8::Local::new(scope, &info.handle)),
-        None => {
-          let msg = format!(
-            r#"Cannot resolve module "{}" from "{}""#,
-            req_str, referrer_name
-          );
-          throw_type_error(scope, msg);
-          return None;
-        }
-      }
+  if let Some(id) = state.modules.get_id(resolved_specifier.as_str()) {
+    if let Some(handle) = state.modules.get_handle(id) {
+      return Some(v8::Local::new(scope, handle));
     }
   }
 
+  let msg = format!(
+    r#"Cannot resolve module "{}" from "{}""#,
+    specifier_str, referrer_name
+  );
+  throw_type_error(scope, msg);
   None
 }
 

core/modules.rs

@@ -337,9 +337,9 @@ impl Stream for RecursiveModuleLoad {
 }
 
 pub struct ModuleInfo {
+  pub id: ModuleId,
   pub main: bool,
   pub name: String,
-  pub handle: v8::Global<v8::Module>,
   pub import_specifiers: Vec<ModuleSpecifier>,
 }
 
@@ -372,17 +372,12 @@ impl ModuleNameMap {
   pub fn get(&self, name: &str) -> Option<ModuleId> {
     let mut mod_name = name;
     loop {
-      let cond = self.inner.get(mod_name);
-      match cond {
-        Some(SymbolicModule::Alias(target)) => {
+      let symbolic_module = self.inner.get(mod_name)?;
+      match symbolic_module {
+        SymbolicModule::Alias(target) => {
           mod_name = target;
         }
-        Some(SymbolicModule::Mod(mod_id)) => {
-          return Some(*mod_id);
-        }
-        _ => {
-          return None;
-        }
+        SymbolicModule::Mod(mod_id) => return Some(*mod_id),
       }
     }
   }
@@ -408,15 +403,21 @@ impl ModuleNameMap {
 /// A collection of JS modules.
 #[derive(Default)]
 pub struct Modules {
-  pub(crate) info: HashMap<ModuleId, ModuleInfo>,
+  ids_by_handle: HashMap<v8::Global<v8::Module>, ModuleId>,
+  handles_by_id: HashMap<ModuleId, v8::Global<v8::Module>>,
+  info: HashMap<ModuleId, ModuleInfo>,
   by_name: ModuleNameMap,
+  next_module_id: ModuleId,
 }
 
 impl Modules {
   pub fn new() -> Modules {
     Self {
+      handles_by_id: HashMap::new(),
+      ids_by_handle: HashMap::new(),
       info: HashMap::new(),
       by_name: ModuleNameMap::new(),
+      next_module_id: 1,
     }
   }
 
@@ -428,35 +429,33 @@ impl Modules {
     self.info.get(&id).map(|i| &i.import_specifiers)
   }
 
-  pub fn get_name(&self, id: ModuleId) -> Option<&String> {
-    self.info.get(&id).map(|i| &i.name)
-  }
-
   pub fn is_registered(&self, specifier: &ModuleSpecifier) -> bool {
     self.by_name.get(&specifier.to_string()).is_some()
   }
 
   pub fn register(
     &mut self,
-    id: ModuleId,
     name: &str,
     main: bool,
     handle: v8::Global<v8::Module>,
     import_specifiers: Vec<ModuleSpecifier>,
-  ) {
+  ) -> ModuleId {
     let name = String::from(name);
-    debug!("register_complete {}", name);
-
+    let id = self.next_module_id;
+    self.next_module_id += 1;
     self.by_name.insert(name.clone(), id);
+    self.handles_by_id.insert(id, handle.clone());
+    self.ids_by_handle.insert(handle, id);
     self.info.insert(
       id,
       ModuleInfo {
+        id,
         main,
         name,
         import_specifiers,
-        handle,
       },
     );
+    id
   }
 
   pub fn alias(&mut self, name: &str, target: &str) {
@@ -468,11 +467,19 @@ impl Modules {
     self.by_name.is_alias(name)
   }
 
-  pub fn get_info(&self, id: ModuleId) -> Option<&ModuleInfo> {
-    if id == 0 {
-      return None;
+  pub fn get_handle(&self, id: ModuleId) -> Option<v8::Global<v8::Module>> {
+    self.handles_by_id.get(&id).cloned()
+  }
+
+  pub fn get_info(
+    &self,
+    global: &v8::Global<v8::Module>,
+  ) -> Option<&ModuleInfo> {
+    if let Some(id) = self.ids_by_handle.get(global) {
+      return self.info.get(id);
     }
-    self.info.get(&id)
+
+    None
   }
 }
 

core/runtime.rs

@@ -114,7 +114,7 @@ pub(crate) struct JsRuntimeState {
   pub(crate) have_unpolled_ops: Cell<bool>,
   //pub(crate) op_table: OpTable,
   pub(crate) op_state: Rc<RefCell<OpState>>,
-  loader: Rc<dyn ModuleLoader>,
+  pub loader: Rc<dyn ModuleLoader>,
   pub modules: Modules,
   pub(crate) dyn_import_map:
     HashMap<ModuleLoadId, v8::Global<v8::PromiseResolver>>,
@@ -572,20 +572,6 @@ where
 }
 
 impl JsRuntimeState {
-  // Called by V8 during `Isolate::mod_instantiate`.
-  pub fn module_resolve_cb(
-    &mut self,
-    specifier: &str,
-    referrer_id: ModuleId,
-  ) -> ModuleId {
-    let referrer = self.modules.get_name(referrer_id).unwrap();
-    let specifier = self
-      .loader
-      .resolve(self.op_state.clone(), specifier, referrer, false)
-      .expect("Module should have been already resolved");
-    self.modules.get_id(specifier.as_str()).unwrap_or(0)
-  }
-
   // Called by V8 during `Isolate::mod_instantiate`.
   pub fn dyn_import_cb(
     &mut self,
@@ -687,7 +673,6 @@ impl JsRuntime {
     }
 
     let module = maybe_module.unwrap();
-    let id = module.get_identity_hash();
 
     let mut import_specifiers: Vec<ModuleSpecifier> = vec![];
     for i in 0..module.get_module_requests_length() {
@@ -703,8 +688,7 @@ impl JsRuntime {
       import_specifiers.push(module_specifier);
     }
 
-    state_rc.borrow_mut().modules.register(
-      id,
+    let id = state_rc.borrow_mut().modules.register(
       name,
       main,
       v8::Global::<v8::Module>::new(tc_scope, module),
@@ -726,13 +710,12 @@ impl JsRuntime {
     let scope = &mut v8::HandleScope::with_context(self.v8_isolate(), context);
     let tc_scope = &mut v8::TryCatch::new(scope);
 
-    let state = state_rc.borrow();
-    let module = match state.modules.get_info(id) {
-      Some(info) => v8::Local::new(tc_scope, &info.handle),
-      None if id == 0 => return Ok(()),
-      _ => panic!("module id {} not found in module table", id),
-    };
-    drop(state);
+    let module = state_rc
+      .borrow()
+      .modules
+      .get_handle(id)
+      .map(|handle| v8::Local::new(tc_scope, handle))
+      .expect("ModuleInfo not found");
 
     if module.get_status() == v8::ModuleStatus::Errored {
       exception_to_err_result(tc_scope, module.get_exception(), false)?
@@ -768,10 +751,8 @@ impl JsRuntime {
     let module_handle = state_rc
       .borrow()
       .modules
-      .get_info(id)
-      .expect("ModuleInfo not found")
-      .handle
-      .clone();
+      .get_handle(id)
+      .expect("ModuleInfo not found");
 
     let status = {
       let scope =
@@ -858,8 +839,8 @@ impl JsRuntime {
     let module = state_rc
       .borrow()
       .modules
-      .get_info(id)
-      .map(|info| v8::Local::new(scope, &info.handle))
+      .get_handle(id)
+      .map(|handle| v8::Local::new(scope, handle))
       .expect("ModuleInfo not found");
     let mut status = module.get_status();
 
@@ -970,7 +951,6 @@ impl JsRuntime {
     let context = self.global_context();
 
     debug!("dyn_import_done {} {:?}", id, mod_id);
-    assert!(mod_id != 0);
     let scope = &mut v8::HandleScope::with_context(self.v8_isolate(), context);
 
     let resolver_handle = state_rc
@@ -984,8 +964,8 @@ impl JsRuntime {
       let state = state_rc.borrow();
       state
         .modules
-        .get_info(mod_id)
-        .map(|info| v8::Local::new(scope, &info.handle))
+        .get_handle(mod_id)
+        .map(|handle| v8::Local::new(scope, handle))
         .expect("Dyn import module info not found")
     };
     // Resolution success