refactor: Untangle "OpCtx" and "ContextState" (denoland#506)

This commit removes entanglement between "OpCtx" and "ContextState"
structs.

"OpCtx" no longer requires "ContextState" to be passed as an argument.
Instead it accepts "op_driver" arg which is "OpDriverImpl" (renamed from
"DefaultOpDriver").

This allows us to fully set up "OpCtx" for every op and only then create
"ContextState".
This unlocks a lock of further clean ups that are done in
denoland#499.

core/ops.rs

@@ -5,8 +5,8 @@ use crate::error::GetErrorClassFn;
 use crate::gotham_state::GothamState;
 use crate::io::ResourceTable;
 use crate::ops_metrics::OpMetricsFn;
-use crate::runtime::ContextState;
 use crate::runtime::JsRuntimeState;
+use crate::runtime::OpDriverImpl;
 use crate::FeatureChecker;
 use crate::OpDecl;
 use futures::task::AtomicWaker;
@@ -72,13 +72,13 @@ pub struct OpCtx {
   #[doc(hidden)]
   pub get_error_class_fn: GetErrorClassFn,
 
-  pub(crate) decl: Rc<OpDecl>,
+  pub(crate) decl: OpDecl,
   pub(crate) fast_fn_c_info: Option<NonNull<v8::fast_api::CFunctionInfo>>,
   pub(crate) metrics_fn: Option<OpMetricsFn>,
   /// If the last fast op failed, stores the error to be picked up by the slow op.
   pub(crate) last_fast_error: UnsafeCell<Option<AnyError>>,
 
-  context_state: Rc<ContextState>,
+  op_driver: Rc<OpDriverImpl>,
   runtime_state: Rc<JsRuntimeState>,
 }
 
@@ -87,8 +87,8 @@ impl OpCtx {
   pub(crate) fn new(
     id: OpId,
     isolate: *mut Isolate,
-    context_state: Rc<ContextState>,
-    decl: Rc<OpDecl>,
+    op_driver: Rc<OpDriverImpl>,
+    decl: OpDecl,
     state: Rc<RefCell<OpState>>,
     runtime_state: Rc<JsRuntimeState>,
     get_error_class_fn: GetErrorClassFn,
@@ -129,7 +129,7 @@ impl OpCtx {
       get_error_class_fn,
       runtime_state,
       decl,
-      context_state,
+      op_driver,
       fast_fn_c_info,
       last_fast_error: UnsafeCell::new(None),
       isolate,
@@ -173,9 +173,8 @@ impl OpCtx {
     *opt_mut = Some(error);
   }
 
-  /// Get the [`ContextState`] for this op.
-  pub(crate) fn context_state(&self) -> &ContextState {
-    &self.context_state
+  pub(crate) fn op_driver(&self) -> &OpDriverImpl {
+    &self.op_driver
   }
 
   /// Get the [`JsRuntimeState`] for this op.

core/ops_builtin_v8.rs

@@ -853,8 +853,11 @@ pub fn op_dispatch_exception(
 #[serde]
 pub fn op_op_names(scope: &mut v8::HandleScope) -> Vec<String> {
   let state = JsRealm::state_from_scope(scope);
-  let ctx = state.op_ctxs.borrow();
-  ctx.iter().map(|o| o.decl.name.to_string()).collect()
+  state
+    .op_ctxs
+    .iter()
+    .map(|o| o.decl.name.to_string())
+    .collect()
 }
 
 #[derive(Deserialize, Serialize)]

core/runtime/jsrealm.rs

@@ -1,6 +1,7 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 use super::bindings;
 use super::exception_state::ExceptionState;
+#[cfg(test)]
 use super::op_driver::OpDriver;
 use crate::error::exception_to_err_result;
 use crate::module_specifier::ModuleSpecifier;
@@ -48,12 +49,12 @@ impl Hasher for IdentityHasher {
   feature = "op_driver_joinset",
   not(feature = "op_driver_futuresunordered")
 ))]
-type DefaultOpDriver = super::op_driver::JoinSetDriver;
+pub(crate) type OpDriverImpl = super::op_driver::JoinSetDriver;
 
 #[cfg(feature = "op_driver_futuresunordered")]
-type DefaultOpDriver = super::op_driver::FuturesUnorderedDriver;
+pub(crate) type OpDriverImpl = super::op_driver::FuturesUnorderedDriver;
 
-pub(crate) struct ContextState<OpDriverImpl: OpDriver = DefaultOpDriver> {
+pub(crate) struct ContextState {
   pub(crate) task_spawner_factory: Arc<V8TaskSpawnerFactory>,
   pub(crate) timers: WebTimers<(v8::Global<v8::Function>, u32)>,
   pub(crate) js_event_loop_tick_cb:
@@ -62,33 +63,32 @@ pub(crate) struct ContextState<OpDriverImpl: OpDriver = DefaultOpDriver> {
     RefCell<Option<Rc<v8::Global<v8::Function>>>>,
   pub(crate) unrefed_ops:
     RefCell<HashSet<i32, BuildHasherDefault<IdentityHasher>>>,
-  pub(crate) pending_ops: OpDriverImpl,
+  pub(crate) pending_ops: Rc<OpDriverImpl>,
   // We don't explicitly re-read this prop but need the slice to live alongside
   // the context
-  pub(crate) op_ctxs: RefCell<Box<[OpCtx]>>,
+  pub(crate) op_ctxs: Box<[OpCtx]>,
   pub(crate) isolate: Option<*mut v8::OwnedIsolate>,
   pub(crate) exception_state: Rc<ExceptionState>,
   pub(crate) has_next_tick_scheduled: Cell<bool>,
   pub(crate) get_error_class_fn: GetErrorClassFn,
-  pub(crate) ops_with_metrics: Vec<bool>,
 }
 
-impl<O: OpDriver> ContextState<O> {
+impl ContextState {
   pub(crate) fn new(
+    op_driver: Rc<OpDriverImpl>,
     isolate_ptr: *mut v8::OwnedIsolate,
     get_error_class_fn: GetErrorClassFn,
-    ops_with_metrics: Vec<bool>,
+    op_ctxs: Box<[OpCtx]>,
   ) -> Self {
     Self {
       isolate: Some(isolate_ptr),
       get_error_class_fn,
-      ops_with_metrics,
       exception_state: Default::default(),
       has_next_tick_scheduled: Default::default(),
       js_event_loop_tick_cb: Default::default(),
       js_wasm_streaming_cb: Default::default(),
-      op_ctxs: Default::default(),
-      pending_ops: Default::default(),
+      op_ctxs,
+      pending_ops: op_driver,
       task_spawner_factory: Default::default(),
       timers: Default::default(),
       unrefed_ops: Default::default(),
@@ -181,8 +181,6 @@ impl JsRealmInner {
     state.exception_state.prepare_to_destroy();
     std::mem::take(&mut *state.js_event_loop_tick_cb.borrow_mut());
     std::mem::take(&mut *state.js_wasm_streaming_cb.borrow_mut());
-    // The OpCtx slice may contain a circular reference
-    std::mem::take(&mut *state.op_ctxs.borrow_mut());
 
     self.context().open(isolate).clear_all_slots(isolate);
 

core/runtime/jsruntime.rs

@@ -35,6 +35,7 @@ use crate::ops_metrics::dispatch_metrics_async;
 use crate::ops_metrics::OpMetricsFactoryFn;
 use crate::runtime::ContextState;
 use crate::runtime::JsRealm;
+use crate::runtime::OpDriverImpl;
 use crate::source_map::SourceMapCache;
 use crate::source_map::SourceMapGetter;
 use crate::Extension;
@@ -713,49 +714,23 @@ impl JsRuntime {
       waker,
     });
 
-    let count = ops.len();
-    let op_metrics_fns = ops
-      .iter()
-      .enumerate()
-      .map(|(id, decl)| {
-        options
-          .op_metrics_factory_fn
-          .as_ref()
-          .and_then(|f| (f)(id as _, count, decl))
-      })
-      .collect::<Vec<_>>();
-    let ops_with_metrics = op_metrics_fns
-      .iter()
-      .map(|o| o.is_some())
-      .collect::<Vec<_>>();
-    let context_state = Rc::new(ContextState::new(
-      isolate_ptr,
-      options.get_error_class_fn.unwrap_or(&|_| "Error"),
-      ops_with_metrics,
-    ));
-
-    // Add the task spawners to the OpState
-    let spawner = context_state
-      .task_spawner_factory
-      .clone()
-      .new_same_thread_spawner();
-    op_state.borrow_mut().put(spawner);
-    let spawner = context_state
-      .task_spawner_factory
-      .clone()
-      .new_cross_thread_spawner();
-    op_state.borrow_mut().put(spawner);
+    let op_count = ops.len();
+    let op_driver = Rc::new(OpDriverImpl::default());
+    let op_metrics_factory_fn = options.op_metrics_factory_fn.take();
 
     let mut op_ctxs = ops
       .into_iter()
       .enumerate()
-      .zip(op_metrics_fns)
-      .map(|((id, decl), metrics_fn)| {
+      .map(|(id, decl)| {
+        let metrics_fn = op_metrics_factory_fn
+          .as_ref()
+          .and_then(|f| (f)(id as _, op_count, &decl));
+
         OpCtx::new(
           id as _,
           std::ptr::null_mut(),
-          context_state.clone(),
-          Rc::new(decl),
+          op_driver.clone(),
+          decl,
           op_state.clone(),
           state_rc.clone(),
           options.get_error_class_fn.unwrap_or(&|_| "Error"),
@@ -816,7 +791,25 @@ impl JsRuntime {
     for op_ctx in op_ctxs.iter_mut() {
       op_ctx.isolate = isolate.as_mut() as *mut Isolate;
     }
-    *context_state.op_ctxs.borrow_mut() = op_ctxs;
+
+    let context_state = Rc::new(ContextState::new(
+      op_driver.clone(),
+      isolate_ptr,
+      options.get_error_class_fn.unwrap_or(&|_| "Error"),
+      op_ctxs,
+    ));
+
+    // Add the task spawners to the OpState
+    let spawner = context_state
+      .task_spawner_factory
+      .clone()
+      .new_same_thread_spawner();
+    op_state.borrow_mut().put(spawner);
+    let spawner = context_state
+      .task_spawner_factory
+      .clone()
+      .new_cross_thread_spawner();
+    op_state.borrow_mut().put(spawner);
 
     isolate.set_capture_stack_trace_for_uncaught_exceptions(true, 10);
     isolate.set_promise_reject_callback(bindings::promise_reject_callback);
@@ -865,7 +858,7 @@ impl JsRuntime {
     bindings::initialize_context(
       scope,
       context,
-      &context_state.op_ctxs.borrow(),
+      &context_state.op_ctxs,
       init_mode,
     );
 
@@ -1060,10 +1053,12 @@ impl JsRuntime {
         let scope = &mut self.handle_scope();
         let context_local = v8::Local::new(scope, context_global);
         let context_state = JsRealm::state_from_scope(scope);
-        let op_ctxs = context_state.op_ctxs.borrow();
         let global = context_local.global(scope);
-        let synthetic_module_exports =
-          get_exports_for_ops_virtual_module(&op_ctxs, scope, global);
+        let synthetic_module_exports = get_exports_for_ops_virtual_module(
+          &context_state.op_ctxs,
+          scope,
+          global,
+        );
         let mod_id = module_map
           .new_synthetic_module(
             scope,
@@ -1347,14 +1342,6 @@ impl JsRuntime {
     self.inner.state.op_state.clone()
   }
 
-  /// Returns the runtime's op names, ordered by OpId.
-  pub fn op_names(&self) -> Vec<&'static str> {
-    let main_realm = self.inner.main_realm.clone();
-    let state_rc = main_realm.0.state();
-    let ctx = state_rc.op_ctxs.borrow();
-    ctx.iter().map(|o| o.decl.name).collect()
-  }
-
   /// Executes traditional JavaScript code (traditional = not ES modules).
   ///
   /// The execution takes place on the current main realm, so it is possible
@@ -2272,17 +2259,14 @@ impl JsRuntime {
 
       let res = res.unwrap(scope, context_state.get_error_class_fn);
 
-      if context_state.ops_with_metrics[op_id as usize] {
-        if res.is_ok() {
-          dispatch_metrics_async(
-            &context_state.op_ctxs.borrow()[op_id as usize],
-            OpMetricsEvent::CompletedAsync,
-          );
-        } else {
-          dispatch_metrics_async(
-            &context_state.op_ctxs.borrow()[op_id as usize],
-            OpMetricsEvent::ErrorAsync,
-          );
+      {
+        let op_ctx = &context_state.op_ctxs[op_id as usize];
+        if op_ctx.metrics_enabled() {
+          if res.is_ok() {
+            dispatch_metrics_async(op_ctx, OpMetricsEvent::CompletedAsync);
+          } else {
+            dispatch_metrics_async(op_ctx, OpMetricsEvent::ErrorAsync);
+          }
         }
       }
 

core/runtime/mod.rs

@@ -18,6 +18,7 @@ pub const V8_WRAPPER_OBJECT_INDEX: i32 = 1;
 
 pub(crate) use jsrealm::ContextState;
 pub(crate) use jsrealm::JsRealm;
+pub(crate) use jsrealm::OpDriverImpl;
 pub use jsruntime::CompiledWasmModuleStore;
 pub use jsruntime::CreateRealmOptions;
 pub use jsruntime::CrossIsolateStore;

core/runtime/ops.rs

@@ -40,8 +40,7 @@ pub fn map_async_op_infallible<R: 'static>(
   op: impl Future<Output = R> + 'static,
   rv_map: V8RetValMapper<R>,
 ) -> Option<R> {
-  let pending_ops = &ctx.context_state().pending_ops;
-  pending_ops.submit_op_infallible_scheduling(
+  ctx.op_driver().submit_op_infallible_scheduling(
     op_scheduling(lazy, deferred),
     ctx.id,
     promise_id,
@@ -59,8 +58,7 @@ pub fn map_async_op_fallible<R: 'static, E: Into<Error> + 'static>(
   op: impl Future<Output = Result<R, E>> + 'static,
   rv_map: V8RetValMapper<R>,
 ) -> Option<Result<R, E>> {
-  let pending_ops = &ctx.context_state().pending_ops;
-  pending_ops.submit_op_fallible_scheduling(
+  ctx.op_driver().submit_op_fallible_scheduling(
     op_scheduling(lazy, deferred),
     ctx.id,
     promise_id,

core/runtime/stats.rs

@@ -187,7 +187,7 @@ impl RuntimeActivityStats {
         + self.resources.resources.len()
         + self.timers.timers.len(),
     );
-    let ops = self.context_state.op_ctxs.borrow();
+    let ops = &self.context_state.op_ctxs;
     for op in self.ops.ops.iter() {
       v.push(RuntimeActivity::AsyncOp(op.0, ops[op.1 as usize].decl.name));
     }
@@ -207,7 +207,7 @@ impl RuntimeActivityStats {
   pub fn diff(before: &Self, after: &Self) -> RuntimeActivityDiff {
     let mut appeared = vec![];
     let mut disappeared = vec![];
-    let ops = before.context_state.op_ctxs.borrow();
+    let ops = &before.context_state.op_ctxs;
 
     let mut a = BitSet::new();
     for op in after.ops.ops.iter() {