Repository: denoland/deno_core
Pull request #1012 (2024-12-22 05:57:21)

Remove intermediate OpMethodCtx
It is cheaper to store all OpCtx in one place and pass around OpMethodDecl and an offset that divides the OpCtx in normal ops and method ops.


Patch:
diff --git a/core/extension_set.rs b/core/extension_set.rs
index c799678..8989f34 100644
--- a/core/extension_set.rs
+++ b/core/extension_set.rs
@@ -11,7 +11,6 @@ use crate::extensions::GlobalTemplateMiddlewareFn;
 use crate::extensions::OpMiddlewareFn;
 use crate::modules::ModuleName;
 use crate::ops::OpCtx;
-use crate::ops::OpMethodCtx;
 use crate::runtime::ExtensionTranspiler;
 use crate::runtime::JsRuntimeState;
 use crate::runtime::OpDriverImpl;
@@ -135,17 +134,16 @@ fn check_no_duplicate_op_names(ops: &[OpDecl]) {
 #[allow(clippy::too_many_arguments)]
 pub fn create_op_ctxs(
   op_decls: Vec<OpDecl>,
-  op_method_decls: Vec<OpMethodDecl>,
+  op_method_decls: &mut [OpMethodDecl],
   op_metrics_factory_fn: Option<OpMetricsFactoryFn>,
   op_driver: Rc<OpDriverImpl>,
   op_state: Rc<RefCell<OpState>>,
   runtime_state: Rc<JsRuntimeState>,
   get_error_class_fn: GetErrorClassFn,
   enable_stack_trace_in_ops: bool,
-) -> (Box<[OpCtx]>, Box<[OpMethodCtx]>) {
-  let op_count = op_decls.len();
+) -> (Box<[OpCtx]>, usize) {
+  let op_count = op_decls.len() + op_method_decls.len();
   let mut op_ctxs = Vec::with_capacity(op_count);
-  let mut op_method_ctxs = Vec::with_capacity(op_method_decls.len());
 
   let runtime_state_ptr = runtime_state.as_ref() as *const _;
   let create_ctx = |index, decl| {
@@ -166,34 +164,27 @@ pub fn create_op_ctxs(
     )
   };
 
-  for (index, decl) in op_decls.into_iter().enumerate() {
-    op_ctxs.push(create_ctx(index, decl));
-  }
-
-  for (index, mut decl) in op_method_decls.into_iter().enumerate() {
+  for (index, decl) in op_method_decls.iter_mut().enumerate() {
     decl.constructor.name = decl.name.0;
     decl.constructor.name_fast = decl.name.1;
 
-    op_method_ctxs.push(OpMethodCtx {
-      type_name: (decl.type_name)(),
-      constructor: create_ctx(index, decl.constructor),
-      methods: decl
-        .methods
-        .iter()
-        .map(|method_decl| create_ctx(index, *method_decl))
-        .collect(),
-      static_methods: decl
-        .static_methods
-        .iter()
-        .map(|method_decl| create_ctx(index, *method_decl))
-        .collect(),
-    });
+    op_ctxs.push(create_ctx(index, decl.constructor));
+    for method in decl.methods {
+      op_ctxs.push(create_ctx(index, *method));
+    }
+    for method in decl.static_methods {
+      op_ctxs.push(create_ctx(index, *method));
+    }
   }
 
-  (
-    op_ctxs.into_boxed_slice(),
-    op_method_ctxs.into_boxed_slice(),
-  )
+  /* method op ctxs are stored before regular op ctxs */
+  let methods_ctx_offset = op_ctxs.len();
+
+  for (index, decl) in op_decls.into_iter().enumerate() {
+    op_ctxs.push(create_ctx(index + methods_ctx_offset, decl));
+  }
+
+  (op_ctxs.into_boxed_slice(), methods_ctx_offset)
 }
 
 pub fn get_middlewares_and_external_refs(
diff --git a/core/ops.rs b/core/ops.rs
index 6bc5e5f..57e8e5c 100644
--- a/core/ops.rs
+++ b/core/ops.rs
@@ -71,17 +71,6 @@ impl OpMetadata {
   }
 }
 
-/// Per-object contexts for members.
-pub struct OpMethodCtx {
-  pub type_name: &'static str,
-  /// Op context for the constructor
-  pub constructor: OpCtx,
-  /// Per-op context for the methods
-  pub methods: Vec<OpCtx>,
-  /// Per-op context for the static methods
-  pub static_methods: Vec<OpCtx>,
-}
-
 /// Per-op context.
 ///
 // Note: We don't worry too much about the size of this struct because it's allocated once per realm, and is
diff --git a/core/runtime/bindings.rs b/core/runtime/bindings.rs
index e312c9e..53ee8ee 100644
--- a/core/runtime/bindings.rs
+++ b/core/runtime/bindings.rs
@@ -10,6 +10,7 @@ use v8::MapFnTo;
 use super::jsruntime::BUILTIN_SOURCES;
 use super::jsruntime::CONTEXT_SETUP_SOURCES;
 use super::v8_static_strings::*;
+use crate::_ops::OpMethodDecl;
 use crate::cppgc::cppgc_template_constructor;
 use crate::cppgc::FunctionTemplateData;
 use crate::error::callsite_fns;
@@ -25,7 +26,6 @@ use crate::modules::synthetic_module_evaluation_steps;
 use crate::modules::ImportAttributesKind;
 use crate::modules::ModuleMap;
 use crate::ops::OpCtx;
-use crate::ops::OpMethodCtx;
 use crate::runtime::InitMode;
 use crate::runtime::JsRealm;
 use crate::AccessorType;
@@ -36,7 +36,6 @@ use crate::ModuleType;
 
 pub(crate) fn create_external_references(
   ops: &[OpCtx],
-  op_method_ctxs: &[OpMethodCtx],
   additional_references: &[v8::ExternalReference],
   sources: &[v8::OneByteConst],
   ops_in_snapshot: usize,
@@ -49,7 +48,6 @@ pub(crate) fn create_external_references(
       + (ops.len() * 4)
       + additional_references.len()
       + sources.len()
-      + op_method_ctxs.len()
       + 18, // for callsite_fns
   );
 
@@ -89,16 +87,6 @@ pub(crate) fn create_external_references(
     });
   }
 
-  for ctx in op_method_ctxs {
-    references.extend_from_slice(&ctx.constructor.external_references());
-    for method in &ctx.methods {
-      references.extend_from_slice(&method.external_references());
-    }
-    for method in &ctx.static_methods {
-      references.extend_from_slice(&method.external_references());
-    }
-  }
-
   references.extend_from_slice(additional_references);
 
   for ctx in &ops[..ops_in_snapshot] {
@@ -345,7 +333,8 @@ pub(crate) fn initialize_deno_core_ops_bindings<'s>(
   scope: &mut v8::HandleScope<'s>,
   context: v8::Local<'s, v8::Context>,
   op_ctxs: &[OpCtx],
-  op_method_ctxs: &[OpMethodCtx],
+  op_method_decls: &[OpMethodDecl],
+  methods_ctx_offset: usize,
   fn_template_store: &mut FunctionTemplateData,
 ) {
   let global = context.global(scope);
@@ -366,52 +355,66 @@ pub(crate) fn initialize_deno_core_ops_bindings<'s>(
   );
 
   let undefined = v8::undefined(scope);
-  for op_ctx in op_ctxs {
-    let mut op_fn = op_ctx_function(scope, op_ctx);
-    let key = op_ctx.decl.name_fast.v8_string(scope).unwrap();
+  let mut index = 0;
 
-    // For async ops we need to set them up, by calling `Deno.core.setUpAsyncStub` -
-    // this call will generate an optimized function that binds to the provided
-    // op, while keeping track of promises and error remapping.
-    if op_ctx.decl.is_async {
-      let result = set_up_async_stub_fn
-        .call(scope, undefined.into(), &[key.into(), op_fn.into()])
-        .unwrap();
-      op_fn = result.try_into().unwrap()
+  for decl in op_method_decls {
+    if index == methods_ctx_offset {
+      break;
     }
+    let constructor_ctx = &op_ctxs[index];
 
-    deno_core_ops_obj.set(scope, key.into(), op_fn.into());
-  }
-
-  for op_method_ctx in op_method_ctxs {
-    let tmpl = op_ctx_template(scope, &op_method_ctx.constructor);
+    let tmpl = op_ctx_template(scope, constructor_ctx);
     let prototype = tmpl.prototype_template(scope);
-    let key = op_method_ctx
-      .constructor
-      .decl
-      .name_fast
-      .v8_string(scope)
-      .unwrap();
+    let key = decl.constructor.name_fast.v8_string(scope).unwrap();
 
-    let accessor_store = create_accessor_store(op_method_ctx);
+    index += 1;
 
-    for method in op_method_ctx.methods.iter() {
+    let method_ctxs = &op_ctxs[index..index + decl.methods.len()];
+
+    let accessor_store = create_accessor_store(method_ctxs);
+
+    for method in method_ctxs.iter() {
       op_ctx_template_or_accessor(&accessor_store, scope, prototype, method);
     }
 
-    for method in op_method_ctx.static_methods.iter() {
+    index += decl.methods.len();
+
+    let static_method_ctxs = &op_ctxs[index..index + decl.static_methods.len()];
+    for method in static_method_ctxs.iter() {
       let op_fn = op_ctx_template(scope, method);
       let method_key = method.decl.name_fast;
       tmpl.set(method_key.v8_string(scope).unwrap().into(), op_fn.into());
     }
 
+    index += decl.static_methods.len();
+
     let op_fn = tmpl.get_function(scope).unwrap();
     op_fn.set_name(key);
     deno_core_ops_obj.set(scope, key.into(), op_fn.into());
 
-    let id = op_method_ctx.type_name.to_string();
+    let id = (decl.type_name)().to_string();
     fn_template_store.insert(id, v8::Global::new(scope, tmpl));
   }
+
+  let op_ctxs = &op_ctxs[index..];
+  for op_ctx in op_ctxs {
+    let mut op_fn = op_ctx_function(scope, op_ctx);
+    let key = op_ctx.decl.name_fast.v8_string(scope).unwrap();
+
+    // For async ops we need to set them up, by calling `Deno.core.setUpAsyncStub` -
+    // this call will generate an optimized function that binds to the provided
+    // op, while keeping track of promises and error remapping.
+    if op_ctx.decl.is_async {
+      let result = set_up_async_stub_fn
+        .call(scope, undefined.into(), &[key.into(), op_fn.into()])
+        .unwrap();
+      op_fn = result.try_into().unwrap()
+    }
+
+    deno_core_ops_obj.set(scope, key.into(), op_fn.into());
+
+    index += 1;
+  }
 }
 
 fn op_ctx_template_or_accessor<'s>(
@@ -520,18 +523,17 @@ fn op_ctx_function<'s>(
 
 type AccessorStore<'a> = HashMap<String, (&'a OpCtx, Option<&'a OpCtx>)>;
 
-fn create_accessor_store(ctx: &OpMethodCtx) -> AccessorStore {
+fn create_accessor_store(method_ctxs: &[OpCtx]) -> AccessorStore {
   let mut store = AccessorStore::new();
 
-  for method in ctx.methods.iter() {
+  for method in method_ctxs.iter() {
     // Populate all setters first.
     if method.decl.accessor_type == AccessorType::Setter {
       let key = method.decl.name_fast;
 
       let key_str = key.to_string();
       // There must be a getter for each setter.
-      let getter = ctx
-        .methods
+      let getter = method_ctxs
         .iter()
         .find(|m| {
           m.decl.name == key_str && m.decl.accessor_type == AccessorType::Getter
@@ -543,7 +545,7 @@ fn create_accessor_store(ctx: &OpMethodCtx) -> AccessorStore {
   }
 
   // Populate getters without setters.
-  for method in ctx.methods.iter() {
+  for method in method_ctxs.iter() {
     if method.decl.accessor_type == AccessorType::Getter {
       let key = method.decl.name_fast.to_string();
 
@@ -971,24 +973,36 @@ where
 /// to a JavaScript function that executes and op.
 pub fn create_exports_for_ops_virtual_module<'s>(
   op_ctxs: &[OpCtx],
-  op_method_ctxs: &[OpMethodCtx],
+  op_method_decls: &[OpMethodDecl],
+  methods_ctx_offset: usize,
   scope: &mut v8::HandleScope<'s>,
   global: v8::Local<v8::Object>,
 ) -> Vec<(FastStaticString, v8::Local<'s, v8::Value>)> {
-  let mut exports = Vec::with_capacity(op_ctxs.len() + op_method_ctxs.len());
+  let mut exports = Vec::with_capacity(op_ctxs.len());
 
   let deno_obj = get(scope, global, DENO, "Deno");
   let deno_core_obj = get(scope, deno_obj, CORE, "Deno.core");
   let ops_obj = get(scope, deno_core_obj, OPS, "Deno.core.ops");
 
+  let mut index = 0;
+
+  for decl in op_method_decls {
+    if index == methods_ctx_offset {
+      break;
+    }
+    let constructor_ctx = &op_ctxs[index];
+
+    let op_fn = get(scope, ops_obj, constructor_ctx.decl.name_fast, "op");
+    exports.push((constructor_ctx.decl.name_fast, op_fn));
+
+    index += 1 + decl.methods.len() + decl.static_methods.len();
+  }
+
+  let op_ctxs = &op_ctxs[index..];
   for op_ctx in op_ctxs {
     let op_fn = get(scope, ops_obj, op_ctx.decl.name_fast, "op");
     exports.push((op_ctx.decl.name_fast, op_fn));
-  }
-
-  for ctx in op_method_ctxs {
-    let op_fn = get(scope, ops_obj, ctx.constructor.decl.name_fast, "op");
-    exports.push((ctx.constructor.decl.name_fast, op_fn));
+    index += 1;
   }
 
   exports
diff --git a/core/runtime/jsrealm.rs b/core/runtime/jsrealm.rs
index ba8132a..fea30d8 100644
--- a/core/runtime/jsrealm.rs
+++ b/core/runtime/jsrealm.rs
@@ -2,6 +2,7 @@
 use super::exception_state::ExceptionState;
 #[cfg(test)]
 use super::op_driver::OpDriver;
+use crate::_ops::OpMethodDecl;
 use crate::cppgc::FunctionTemplateData;
 use crate::error::exception_to_err_result;
 use crate::module_specifier::ModuleSpecifier;
@@ -14,7 +15,6 @@ use crate::modules::ModuleMap;
 use crate::modules::ModuleName;
 use crate::ops::ExternalOpsTracker;
 use crate::ops::OpCtx;
-use crate::ops::OpMethodCtx;
 use crate::stats::RuntimeActivityTraces;
 use crate::tasks::V8TaskSpawnerFactory;
 use crate::web_timeout::WebTimers;
@@ -69,7 +69,8 @@ pub struct ContextState {
   // We don't explicitly re-read this prop but need the slice to live alongside
   // the context
   pub(crate) op_ctxs: Box<[OpCtx]>,
-  pub(crate) op_method_ctxs: Box<[OpMethodCtx]>,
+  pub(crate) op_method_decls: Vec<OpMethodDecl>,
+  pub(crate) methods_ctx_offset: usize,
   pub(crate) isolate: Option<*mut v8::Isolate>,
   pub(crate) exception_state: Rc<ExceptionState>,
   pub(crate) has_next_tick_scheduled: Cell<bool>,
@@ -83,7 +84,8 @@ impl ContextState {
     isolate_ptr: *mut v8::Isolate,
     get_error_class_fn: GetErrorClassFn,
     op_ctxs: Box<[OpCtx]>,
-    op_method_ctxs: Box<[OpMethodCtx]>,
+    op_method_decls: Vec<OpMethodDecl>,
+    methods_ctx_offset: usize,
     external_ops_tracker: ExternalOpsTracker,
   ) -> Self {
     Self {
@@ -96,7 +98,8 @@ impl ContextState {
       wasm_instance_fn: Default::default(),
       activity_traces: Default::default(),
       op_ctxs,
-      op_method_ctxs,
+      op_method_decls,
+      methods_ctx_offset,
       pending_ops: op_driver,
       task_spawner_factory: Default::default(),
       timers: Default::default(),
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index f2ae258..70b5b5e 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -874,16 +874,16 @@ impl JsRuntime {
 
     // ...now we're moving on to ops; set them up, create `OpCtx` for each op
     // and get ready to actually create V8 isolate...
-    let (op_decls, op_method_decls) =
+    let (op_decls, mut op_method_decls) =
       extension_set::init_ops(crate::ops_builtin::BUILTIN_OPS, &mut extensions);
 
     let op_driver = Rc::new(OpDriverImpl::default());
     let op_metrics_factory_fn = options.op_metrics_factory_fn.take();
     let get_error_class_fn = options.get_error_class_fn.unwrap_or(&|_| "Error");
 
-    let (mut op_ctxs, mut op_method_ctxs) = extension_set::create_op_ctxs(
+    let (mut op_ctxs, methods_ctx_offset) = extension_set::create_op_ctxs(
       op_decls,
-      op_method_decls,
+      &mut op_method_decls,
       op_metrics_factory_fn,
       op_driver.clone(),
       op_state.clone(),
@@ -932,7 +932,6 @@ impl JsRuntime {
     isolate_allocations.external_refs =
       Some(Box::new(bindings::create_external_references(
         &op_ctxs,
-        &op_method_ctxs,
         &additional_references,
         &isolate_allocations.externalized_sources,
         ops_in_snapshot,
@@ -967,12 +966,6 @@ impl JsRuntime {
     for op_ctx in op_ctxs.iter_mut() {
       op_ctx.isolate = isolate_ptr;
     }
-    for op_method_ctx in op_method_ctxs.iter_mut() {
-      op_method_ctx.constructor.isolate = isolate_ptr;
-      for op in &mut op_method_ctx.methods {
-        op.isolate = isolate_ptr;
-      }
-    }
 
     op_state.borrow_mut().put(isolate_ptr);
 
@@ -982,7 +975,8 @@ impl JsRuntime {
       isolate_ptr,
       options.get_error_class_fn.unwrap_or(&|_| "Error"),
       op_ctxs,
-      op_method_ctxs,
+      op_method_decls,
+      methods_ctx_offset,
       op_state.borrow().external_ops_tracker.clone(),
     ));
 
@@ -1051,7 +1045,8 @@ impl JsRuntime {
         scope,
         context,
         &context_state.op_ctxs,
-        &context_state.op_method_ctxs,
+        &context_state.op_method_decls,
+        methods_ctx_offset,
         &mut state_rc.function_templates.borrow_mut(),
       );
     }
@@ -1279,7 +1274,8 @@ impl JsRuntime {
     let global = context_local.global(scope);
     let synthetic_module_exports = create_exports_for_ops_virtual_module(
       &context_state.op_ctxs,
-      &context_state.op_method_ctxs,
+      &context_state.op_method_decls,
+      context_state.methods_ctx_offset,
       scope,
       global,
     );


Test patch:
diff --git a/testing/unit/resource_test.ts b/testing/unit/resource_test.ts
index dbda74c..8c9c77a 100644
--- a/testing/unit/resource_test.ts
+++ b/testing/unit/resource_test.ts
@@ -65,8 +65,8 @@ test(async function testCppgcAsync() {
 });
 
 test(function testDomPoint() {
-  const p1 = new DOMPoint(100, 100);
   const p2 = new DOMPoint();
+  const p1 = new DOMPoint(100, 100);
   const p3 = DOMPoint.fromPoint({ x: 200 });
   const p4 = DOMPoint.fromPoint({ x: 0, y: 100, z: 99.9, w: 100 });
   const p5 = p1.fromPoint({ x: 200 });