refactor(core): OpCtx (#14228)

cli/tests/testdata/workers/test.ts

@@ -655,7 +655,7 @@ Deno.test("Worker with invalid permission arg", function () {
         deno: { permissions: { env: "foo" } },
       }),
     TypeError,
-    'Error parsing args at position 1: (deno.permissions.env) invalid value: string "foo", expected "inherit" or boolean or string[]',
+    'Error parsing args at position 0: (deno.permissions.env) invalid value: string "foo", expected "inherit" or boolean or string[]',
   );
 });
 

core/01_core.js

@@ -15,7 +15,6 @@
     ArrayPrototypeMap,
     ErrorCaptureStackTrace,
     Promise,
-    ObjectEntries,
     ObjectFromEntries,
     MapPrototypeGet,
     MapPrototypeDelete,
@@ -27,10 +26,6 @@
     SymbolFor,
   } = window.__bootstrap.primordials;
   const ops = window.Deno.core.ops;
-  const opIds = Object.keys(ops).reduce((a, v, i) => {
-    a[v] = i;
-    return a;
-  }, {});
 
   // Available on start due to bindings.
   const { refOp_, unrefOp_ } = window.Deno.core;
@@ -154,7 +149,7 @@
 
   function opAsync(opName, ...args) {
     const promiseId = nextPromiseId++;
-    const maybeError = ops[opName](opIds[opName], promiseId, ...args);
+    const maybeError = ops[opName](promiseId, ...args);
     // Handle sync error (e.g: error parsing args)
     if (maybeError) return unwrapOpResult(maybeError);
     let p = PromisePrototypeThen(setPromise(promiseId), unwrapOpResult);
@@ -174,7 +169,7 @@
   }
 
   function opSync(opName, ...args) {
-    return unwrapOpResult(ops[opName](opIds[opName], ...args));
+    return unwrapOpResult(ops[opName](...args));
   }
 
   function refOp(promiseId) {
@@ -222,8 +217,8 @@
   function metrics() {
     const [aggregate, perOps] = opSync("op_metrics");
     aggregate.ops = ObjectFromEntries(ArrayPrototypeMap(
-      ObjectEntries(opIds),
+      core.op_names,
-      ([opName, opId]) => [opName, perOps[opId]],
+      (opName, opId) => [opName, perOps[opId]],
     ));
     return aggregate;
   }

core/bindings.rs

@@ -1,16 +1,15 @@
 // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.
 
 use crate::error::is_instance_of_error;
-use crate::extensions::OpDecl;
 use crate::modules::get_module_type_from_assertions;
 use crate::modules::parse_import_assertions;
 use crate::modules::validate_import_assertions;
 use crate::modules::ImportAssertionsKind;
 use crate::modules::ModuleMap;
+use crate::ops::OpCtx;
 use crate::ops_builtin::WasmStreamingResource;
 use crate::resolve_url_or_path;
 use crate::JsRuntime;
-use crate::OpState;
 use crate::PromiseId;
 use crate::ResourceId;
 use crate::ZeroCopyBuf;
@@ -23,7 +22,6 @@ use serde_v8::to_v8;
 use std::cell::RefCell;
 use std::option::Option;
 use std::os::raw::c_void;
-use std::rc::Rc;
 use url::Url;
 use v8::HandleScope;
 use v8::Local;
@@ -147,9 +145,8 @@ pub fn module_origin<'a>(
 
 pub fn initialize_context<'s>(
   scope: &mut v8::HandleScope<'s, ()>,
-  ops: &[OpDecl],
+  op_ctxs: &[OpCtx],
   snapshot_loaded: bool,
-  op_state: Rc<RefCell<OpState>>,
 ) -> v8::Local<'s, v8::Context> {
   let scope = &mut v8::EscapableHandleScope::new(scope);
 
@@ -165,14 +162,13 @@ pub fn initialize_context<'s>(
   // a really weird usecase. Remove this once all
   // tsc ops are static at snapshot time.
   if snapshot_loaded {
-    // Grab Deno.core.ops object
+    // Grab the Deno.core & Deno.core.ops objects
+    let core_obj = JsRuntime::grab_global::<v8::Object>(scope, "Deno.core")
+      .expect("Deno.core to exist");
     let ops_obj = JsRuntime::grab_global::<v8::Object>(scope, "Deno.core.ops")
       .expect("Deno.core.ops to exist");
-
+    initialize_ops(scope, ops_obj, op_ctxs);
-    let raw_op_state = Rc::as_ptr(&op_state) as *const c_void;
+    initialize_op_names(scope, core_obj, op_ctxs);
-    for op in ops {
-      set_func_raw(scope, ops_obj, op.name, op.v8_fn_ptr, raw_op_state);
-    }
     return scope.escape(context);
   }
 
@@ -236,14 +232,34 @@ pub fn initialize_context<'s>(
   set_func(scope, global, "queueMicrotask", queue_microtask);
 
   // Bind functions to Deno.core.ops.*
-  let ops_val = JsRuntime::ensure_objs(scope, global, "Deno.core.ops").unwrap();
+  let ops_obj = JsRuntime::ensure_objs(scope, global, "Deno.core.ops").unwrap();
-  let raw_op_state = Rc::as_ptr(&op_state) as *const c_void;
+  initialize_ops(scope, ops_obj, op_ctxs);
-  for op in ops {
+  initialize_op_names(scope, core_val, op_ctxs);
-    set_func_raw(scope, ops_val, op.name, op.v8_fn_ptr, raw_op_state);
-  }
   scope.escape(context)
 }
 
+fn initialize_ops(
+  scope: &mut v8::HandleScope,
+  ops_obj: v8::Local<v8::Object>,
+  op_ctxs: &[OpCtx],
+) {
+  for ctx in op_ctxs {
+    let ctx_ptr = ctx as *const OpCtx as *const c_void;
+    set_func_raw(scope, ops_obj, ctx.decl.name, ctx.decl.v8_fn_ptr, ctx_ptr);
+  }
+}
+
+fn initialize_op_names(
+  scope: &mut v8::HandleScope,
+  core_obj: v8::Local<v8::Object>,
+  op_ctxs: &[OpCtx],
+) {
+  let names: Vec<&str> = op_ctxs.iter().map(|o| o.decl.name).collect();
+  let k = v8::String::new(scope, "op_names").unwrap().into();
+  let v = serde_v8::to_v8(scope, names).unwrap();
+  core_obj.set(scope, k, v);
+}
+
 pub fn set_func(
   scope: &mut v8::HandleScope<'_>,
   obj: v8::Local<v8::Object>,

core/examples/hello_world.rs

@@ -5,7 +5,6 @@
 use deno_core::op;
 use deno_core::Extension;
 use deno_core::JsRuntime;
-use deno_core::OpState;
 use deno_core::RuntimeOptions;
 
 // This is a hack to make the `#[op]` macro work with

core/lib.rs

@@ -108,6 +108,7 @@ pub mod _ops {
   pub use super::bindings::throw_type_error;
   pub use super::error_codes::get_error_code;
   pub use super::ops::to_op_result;
+  pub use super::ops::OpCtx;
   pub use super::runtime::queue_async_op;
 }
 

core/ops.rs

@@ -3,6 +3,7 @@
 use crate::gotham_state::GothamState;
 use crate::resources::ResourceTable;
 use crate::runtime::GetErrorClassFn;
+use crate::OpDecl;
 use crate::OpsTracker;
 use anyhow::Error;
 use futures::future::maybe_done;
@@ -12,10 +13,12 @@ use futures::ready;
 use futures::task::noop_waker;
 use futures::Future;
 use serde::Serialize;
+use std::cell::RefCell;
 use std::cell::UnsafeCell;
 use std::ops::Deref;
 use std::ops::DerefMut;
 use std::pin::Pin;
+use std::rc::Rc;
 use std::task::Context;
 use std::task::Poll;
 
@@ -134,6 +137,13 @@ pub fn to_op_result<R: Serialize + 'static>(
   }
 }
 
+// TODO(@AaronO): optimize OpCtx(s) mem usage ?
+pub struct OpCtx {
+  pub id: OpId,
+  pub state: Rc<RefCell<OpState>>,
+  pub decl: OpDecl,
+}
+
 /// Maintains the resources and ops inside a JS runtime.
 pub struct OpState {
   pub resource_table: ResourceTable,

core/runtime.rs

@@ -164,6 +164,9 @@ pub(crate) struct JsRuntimeState {
   pub(crate) unrefed_ops: HashSet<i32>,
   pub(crate) have_unpolled_ops: bool,
   pub(crate) op_state: Rc<RefCell<OpState>>,
+  #[allow(dead_code)]
+  // We don't explicitly re-read this prop but need the slice to live alongside the isolate
+  pub(crate) op_ctxs: Box<[OpCtx]>,
   pub(crate) shared_array_buffer_store: Option<SharedArrayBufferStore>,
   pub(crate) compiled_wasm_module_store: Option<CompiledWasmModuleStore>,
   waker: AtomicWaker,
@@ -298,6 +301,16 @@ impl JsRuntime {
     }
 
     let op_state = Rc::new(RefCell::new(op_state));
+    let op_ctxs = ops
+      .into_iter()
+      .enumerate()
+      .map(|(id, decl)| OpCtx {
+        id,
+        state: op_state.clone(),
+        decl,
+      })
+      .collect::<Vec<_>>()
+      .into_boxed_slice();
 
     let global_context;
     let (mut isolate, maybe_snapshot_creator) = if options.will_snapshot {
@@ -309,8 +322,7 @@ impl JsRuntime {
       let mut isolate = JsRuntime::setup_isolate(isolate);
       {
         let scope = &mut v8::HandleScope::new(&mut isolate);
-        let context =
+        let context = bindings::initialize_context(scope, &op_ctxs, false);
-          bindings::initialize_context(scope, &ops, false, op_state.clone());
         global_context = v8::Global::new(scope, context);
         creator.set_default_context(context);
       }
@@ -336,12 +348,8 @@ impl JsRuntime {
       let mut isolate = JsRuntime::setup_isolate(isolate);
       {
         let scope = &mut v8::HandleScope::new(&mut isolate);
-        let context = bindings::initialize_context(
+        let context =
-          scope,
+          bindings::initialize_context(scope, &op_ctxs, snapshot_loaded);
-          &ops,
-          snapshot_loaded,
-          op_state.clone(),
-        );
 
         global_context = v8::Global::new(scope, context);
       }
@@ -374,6 +382,7 @@ impl JsRuntime {
       shared_array_buffer_store: options.shared_array_buffer_store,
       compiled_wasm_module_store: options.compiled_wasm_module_store,
       op_state: op_state.clone(),
+      op_ctxs,
       have_unpolled_ops: false,
       waker: AtomicWaker::new(),
     })));

ops/lib.rs

@@ -138,17 +138,19 @@ fn codegen_v8_async(core: &TokenStream2, f: &syn::ItemFn) -> TokenStream2 {
     quote! {}
   };
   let rust_i0 = if uses_opstate { 1 } else { 0 };
-  let (arg_decls, args_tail) = codegen_args(core, f, rust_i0, 2);
+  let (arg_decls, args_tail) = codegen_args(core, f, rust_i0, 1);
   let type_params = &f.sig.generics.params;
 
   quote! {
     use #core::futures::FutureExt;
-    // SAFETY: Called from Deno.core.opAsync. Which retrieves the index using opId table.
+    // SAFETY: #core guarantees args.data() is a v8 External pointing to an OpCtx for the isolates lifetime
-    let op_id = unsafe {
+    let ctx = unsafe {
-      #core::v8::Local::<#core::v8::Integer>::cast(args.get(0))
+      &*(#core::v8::Local::<#core::v8::External>::cast(args.data().unwrap_unchecked()).value()
-    }.value() as usize;
+      as *const #core::_ops::OpCtx)
+    };
+    let op_id = ctx.id;
 
-    let promise_id = args.get(1);
+    let promise_id = args.get(0);
     let promise_id = #core::v8::Local::<#core::v8::Integer>::try_from(promise_id)
       .map(|l| l.value() as #core::PromiseId)
       .map_err(#core::anyhow::Error::from);
@@ -163,18 +165,8 @@ fn codegen_v8_async(core: &TokenStream2, f: &syn::ItemFn) -> TokenStream2 {
 
     #arg_decls
 
-    // SAFETY: Unchecked cast to external since #core guarantees args.data() is a v8 External.
+    let state = ctx.state.clone();
-    let state_refcell_raw = unsafe {
-      #core::v8::Local::<#core::v8::External>::cast(args.data().unwrap_unchecked())
-    }.value();
 
-    // SAFETY: The Rc<RefCell<OpState>> is functionally pinned and is tied to the isolate's lifetime
-    let state = unsafe {
-      let ptr = state_refcell_raw as *const std::cell::RefCell<#core::OpState>;
-      // Increment so it will later be decremented/dropped by the underlaying func it is moved to
-      std::rc::Rc::increment_strong_count(ptr);
-      std::rc::Rc::from_raw(ptr)
-    };
     // Track async call & get copy of get_error_class_fn
     let get_class = {
       let state = state.borrow();
@@ -199,30 +191,23 @@ fn codegen_v8_sync(core: &TokenStream2, f: &syn::ItemFn) -> TokenStream2 {
     quote! {}
   };
   let rust_i0 = if uses_opstate { 1 } else { 0 };
-  let (arg_decls, args_tail) = codegen_args(core, f, rust_i0, 1);
+  let (arg_decls, args_tail) = codegen_args(core, f, rust_i0, 0);
   let ret = codegen_sync_ret(core, &f.sig.output);
   let type_params = &f.sig.generics.params;
 
   quote! {
-    // SAFETY: Called from Deno.core.opSync. Which retrieves the index using opId table.
+    // SAFETY: #core guarantees args.data() is a v8 External pointing to an OpCtx for the isolates lifetime
-    let op_id = unsafe {
+    let ctx = unsafe {
-      #core::v8::Local::<#core::v8::Integer>::cast(args.get(0)).value()
+      &*(#core::v8::Local::<#core::v8::External>::cast(args.data().unwrap_unchecked()).value()
-    } as usize;
+      as *const #core::_ops::OpCtx)
+    };
 
     #arg_decls
 
-    // SAFETY: Unchecked cast to external since #core guarantees args.data() is a v8 External.
+    let op_state = &mut ctx.state.borrow_mut();
-    let state_refcell_raw = unsafe {
-      #core::v8::Local::<#core::v8::External>::cast(args.data().unwrap_unchecked())
-    }.value();
-
-    // SAFETY: The Rc<RefCell<OpState>> is functionally pinned and is tied to the isolate's lifetime
-    let state = unsafe { &*(state_refcell_raw as *const std::cell::RefCell<#core::OpState>) };
-
-    let op_state = &mut state.borrow_mut();
     let result = Self::call::<#type_params>(#args_head #args_tail);
 
-    op_state.tracker.track_sync(op_id);
+    op_state.tracker.track_sync(ctx.id);
 
     #ret
   }