Repository: denoland/deno_core
Pull request #673 (2024-03-22 16:42:47)

feat(op2): support cppgc in async ops
`#[cppgc]` return values were added in https://github.com/denoland/deno_core/commit/a736d868d091406ee20c3fda44bde56def2e73f3 which made 
creation of cppgc backed objects possible without direct access to V8 APIs. 

This patch builds on top of that and adds `#[cppgc]` support for async ops.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 31ceb13..c054448 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -506,7 +506,6 @@ dependencies = [
  "fastrand",
  "futures",
  "libc",
- "log",
  "memoffset",
  "parking_lot",
  "pin-project",
diff --git a/Cargo.toml b/Cargo.toml
index 1e643d8..92d7c24 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -39,7 +39,6 @@ cooked-waker = "5"
 fastrand = "2"
 futures = "0.3.21"
 libc = "0.2.126"
-log = "0.4.17"
 memoffset = ">=0.9"
 num-bigint = { version = "0.4", features = ["rand"] }
 parking_lot = "0.12.0"
diff --git a/core/Cargo.toml b/core/Cargo.toml
index a6d9b9f..8b6f985 100644
--- a/core/Cargo.toml
+++ b/core/Cargo.toml
@@ -34,7 +34,6 @@ deno_ops.workspace = true
 deno_unsync.workspace = true
 futures.workspace = true
 libc.workspace = true
-log.workspace = true
 memoffset.workspace = true
 parking_lot.workspace = true
 pin-project.workspace = true
diff --git a/core/gotham_state.rs b/core/gotham_state.rs
index 06ea5dc..abf54ac 100644
--- a/core/gotham_state.rs
+++ b/core/gotham_state.rs
@@ -3,7 +3,6 @@
 // https://github.com/gotham-rs/gotham/blob/bcbbf8923789e341b7a0e62c59909428ca4e22e2/gotham/src/state/mod.rs
 // Copyright 2017 Gotham Project Developers. MIT license.
 
-use log::trace;
 use std::any::type_name;
 use std::any::Any;
 use std::any::TypeId;
@@ -20,7 +19,6 @@ impl GothamState {
   /// type.
   pub fn put<T: 'static>(&mut self, t: T) {
     let type_id = TypeId::of::<T>();
-    trace!(" inserting record to state for type_id `{:?}`", type_id);
     self.data.insert(type_id, Box::new(t));
   }
 
@@ -33,7 +31,6 @@ impl GothamState {
   /// Tries to borrow a value from the `GothamState` storage.
   pub fn try_borrow<T: 'static>(&self) -> Option<&T> {
     let type_id = TypeId::of::<T>();
-    trace!(" borrowing state data for type_id `{:?}`", type_id);
     self.data.get(&type_id).and_then(|b| b.downcast_ref())
   }
 
@@ -45,7 +42,6 @@ impl GothamState {
   /// Tries to mutably borrow a value from the `GothamState` storage.
   pub fn try_borrow_mut<T: 'static>(&mut self) -> Option<&mut T> {
     let type_id = TypeId::of::<T>();
-    trace!(" mutably borrowing state data for type_id `{:?}`", type_id);
     self.data.get_mut(&type_id).and_then(|b| b.downcast_mut())
   }
 
@@ -57,10 +53,6 @@ impl GothamState {
   /// Tries to move a value out of the `GothamState` storage and return ownership.
   pub fn try_take<T: 'static>(&mut self) -> Option<T> {
     let type_id = TypeId::of::<T>();
-    trace!(
-      " taking ownership from state data for type_id `{:?}`",
-      type_id
-    );
     self
       .data
       .remove(&type_id)
diff --git a/core/modules/map.rs b/core/modules/map.rs
index 7ba5a06..fc8101f 100644
--- a/core/modules/map.rs
+++ b/core/modules/map.rs
@@ -41,7 +41,6 @@ use futures::task::noop_waker_ref;
 use futures::task::AtomicWaker;
 use futures::Future;
 use futures::StreamExt;
-use log::debug;
 use v8::Function;
 use v8::PromiseState;
 
@@ -338,10 +337,6 @@ impl ModuleMap {
     let maybe_module_id = self.get_id(&module_url_found, requested_module_type);
 
     if let Some(module_id) = maybe_module_id {
-      debug!(
-        "Already-registered module fetched again: {:?}",
-        module_url_found
-      );
       return Ok(module_id);
     }
 
diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs
index e664923..4ea0043 100644
--- a/core/ops_builtin_v8.rs
+++ b/core/ops_builtin_v8.rs
@@ -101,12 +101,18 @@ pub fn op_leak_tracing_get<'s>(
   #[smi] kind: u8,
   #[smi] id: i32,
 ) -> v8::Local<'s, v8::Value> {
-  use serde_v8::Serializable;
   let context_state = JsRealm::state_from_scope(scope);
   context_state.activity_traces.get(
     RuntimeActivityType::from_u8(kind),
     id as _,
-    |mut x| x.to_v8(scope).unwrap(),
+    |maybe_str| {
+      if let Some(s) = maybe_str {
+        let v8_str = v8::String::new(scope, s).unwrap();
+        v8_str.into()
+      } else {
+        v8::undefined(scope).into()
+      }
+    },
   )
 }
 
diff --git a/core/runtime/bindings.rs b/core/runtime/bindings.rs
index 5f5065a..6d8e741 100644
--- a/core/runtime/bindings.rs
+++ b/core/runtime/bindings.rs
@@ -1,6 +1,5 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 use anyhow::Context;
-use log::debug;
 use std::mem::MaybeUninit;
 use std::os::raw::c_void;
 use std::path::PathBuf;
@@ -429,10 +428,6 @@ pub fn host_import_module_dynamically_callback<'s>(
     let state = JsRuntime::state_from(scope);
     let module_map_rc = JsRealm::module_map_from(scope);
 
-    debug!(
-      "dyn_import specifier {} referrer {} ",
-      specifier_str, referrer_name_str
-    );
     ModuleMap::load_dynamic_import(
       module_map_rc,
       &specifier_str,
diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs
index bb08de1..748223a 100644
--- a/ops/op2/dispatch_slow.rs
+++ b/ops/op2/dispatch_slow.rs
@@ -782,7 +782,7 @@ pub fn return_value_v8_value(
       quote!(deno_core::_ops::RustToV8Marker::<deno_core::_ops::NumberMarker, _>::from(#result))
     }
     ArgMarker::Cppgc => {
-      quote!(deno_core::cppgc::make_cppgc_object(&mut #scope, #result))
+      quote!(deno_core::cppgc::make_cppgc_object(#scope, #result))
     }
     ArgMarker::None => quote!(#result),
   };
diff --git a/serde_v8/lib.rs b/serde_v8/lib.rs
index 0555724..c34eb0b 100644
--- a/serde_v8/lib.rs
+++ b/serde_v8/lib.rs
@@ -5,7 +5,6 @@ mod keys;
 mod magic;
 mod payload;
 mod ser;
-mod serializable;
 
 pub use de::from_v8;
 pub use de::from_v8_cached;
@@ -29,5 +28,3 @@ pub use magic::Global;
 pub use magic::Value;
 pub use ser::to_v8;
 pub use ser::Serializer;
-pub use serializable::Serializable;
-pub use serializable::SerializablePkg;
diff --git a/serde_v8/serializable.rs b/serde_v8/serializable.rs
deleted file mode 100644
index 71c5676..0000000
--- a/serde_v8/serializable.rs
+++ /dev/null
@@ -1,149 +0,0 @@
-// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
-use std::any::TypeId;
-use std::mem::transmute_copy;
-
-use crate::BigInt;
-use crate::ByteString;
-use crate::ToJsBuffer;
-use crate::U16String;
-
-/// Serializable exists to allow boxing values as "objects" to be serialized later,
-/// this is particularly useful for async op-responses. This trait is a more efficient
-/// replacement for erased-serde that makes less allocations, since it's specific to serde_v8
-/// (and thus doesn't have to have generic outputs, etc...)
-pub trait Serializable {
-  fn to_v8<'a>(
-    &mut self,
-    scope: &mut v8::HandleScope<'a>,
-  ) -> Result<v8::Local<'a, v8::Value>, crate::Error>;
-}
-
-/// Allows all implementors of `serde::Serialize` to implement Serializable
-impl<T: serde::Serialize> Serializable for T {
-  fn to_v8<'a>(
-    &mut self,
-    scope: &mut v8::HandleScope<'a>,
-  ) -> Result<v8::Local<'a, v8::Value>, crate::Error> {
-    crate::to_v8(scope, self)
-  }
-}
-
-/// SerializablePkg exists to provide a fast path for op returns,
-/// allowing them to avoid boxing primitives (ints/floats/bool/unit/...)
-pub enum SerializablePkg {
-  Primitive(Primitive),
-  Serializable(Box<dyn Serializable>),
-}
-
-impl SerializablePkg {
-  pub fn to_v8<'a>(
-    &mut self,
-    scope: &mut v8::HandleScope<'a>,
-  ) -> Result<v8::Local<'a, v8::Value>, crate::Error> {
-    match self {
-      Self::Primitive(x) => crate::to_v8(scope, x),
-      Self::Serializable(x) => x.to_v8(scope),
-    }
-  }
-}
-
-/// Primitive serves as a lightweight serializable wrapper around primitives
-/// so that we can use them for async values
-pub enum Primitive {
-  Unit,
-  Bool(bool),
-  Int8(i8),
-  Int16(i16),
-  Int32(i32),
-  Int64(i64),
-  UInt8(u8),
-  UInt16(u16),
-  UInt32(u32),
-  UInt64(u64),
-  Float32(f32),
-  Float64(f64),
-  String(String),
-  RustToV8Buf(ToJsBuffer),
-  ByteString(ByteString),
-  U16String(U16String),
-  BigInt(BigInt),
-}
-
-impl serde::Serialize for Primitive {
-  fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
-  where
-    S: serde::Serializer,
-  {
-    match self {
-      Self::Unit => ().serialize(s),
-      Self::Bool(x) => x.serialize(s),
-      Self::Int8(x) => x.serialize(s),
-      Self::Int16(x) => x.serialize(s),
-      Self::Int32(x) => x.serialize(s),
-      Self::Int64(x) => x.serialize(s),
-      Self::UInt8(x) => x.serialize(s),
-      Self::UInt16(x) => x.serialize(s),
-      Self::UInt32(x) => x.serialize(s),
-      Self::UInt64(x) => x.serialize(s),
-      Self::Float32(x) => x.serialize(s),
-      Self::Float64(x) => x.serialize(s),
-      Self::String(x) => x.serialize(s),
-      Self::RustToV8Buf(x) => x.serialize(s),
-      Self::ByteString(x) => x.serialize(s),
-      Self::U16String(x) => x.serialize(s),
-      Self::BigInt(x) => x.serialize(s),
-    }
-  }
-}
-
-impl<T: serde::Serialize + 'static> From<T> for SerializablePkg {
-  fn from(x: T) -> Self {
-    #[inline(always)]
-    fn tc<T, U>(src: T) -> U {
-      // SAFETY: the caller has ensured via the TypeId that the T and U types
-      // are the same.
-      let x = unsafe { transmute_copy(&src) };
-      std::mem::forget(src);
-      x
-    }
-
-    let tid = TypeId::of::<T>();
-    if tid == TypeId::of::<()>() {
-      Self::Primitive(Primitive::Unit)
-    } else if tid == TypeId::of::<bool>() {
-      Self::Primitive(Primitive::Bool(tc(x)))
-    } else if tid == TypeId::of::<i8>() {
-      Self::Primitive(Primitive::Int8(tc(x)))
-    } else if tid == TypeId::of::<i16>() {
-      Self::Primitive(Primitive::Int16(tc(x)))
-    } else if tid == TypeId::of::<i32>() {
-      Self::Primitive(Primitive::Int32(tc(x)))
-    } else if tid == TypeId::of::<i64>() {
-      Self::Primitive(Primitive::Int64(tc(x)))
-    } else if tid == TypeId::of::<u8>() {
-      Self::Primitive(Primitive::UInt8(tc(x)))
-    } else if tid == TypeId::of::<u16>() {
-      Self::Primitive(Primitive::UInt16(tc(x)))
-    } else if tid == TypeId::of::<u32>() {
-      Self::Primitive(Primitive::UInt32(tc(x)))
-    } else if tid == TypeId::of::<u64>() {
-      Self::Primitive(Primitive::UInt64(tc(x)))
-    } else if tid == TypeId::of::<f32>() {
-      Self::Primitive(Primitive::Float32(tc(x)))
-    } else if tid == TypeId::of::<f64>() {
-      Self::Primitive(Primitive::Float64(tc(x)))
-    } else if tid == TypeId::of::<String>() {
-      Self::Primitive(Primitive::String(tc(x)))
-    } else if tid == TypeId::of::<ToJsBuffer>() {
-      Self::Primitive(Primitive::RustToV8Buf(tc(x)))
-    } else if tid == TypeId::of::<ByteString>() {
-      Self::Primitive(Primitive::ByteString(tc(x)))
-    } else if tid == TypeId::of::<U16String>() {
-      Self::Primitive(Primitive::U16String(tc(x)))
-    } else if tid == TypeId::of::<BigInt>() {
-      Self::Primitive(Primitive::BigInt(tc(x)))
-    } else {
-      Self::Serializable(Box::new(x))
-    }
-  }
-}


Test patch:
diff --git a/core/runtime/tests/ops.rs b/core/runtime/tests/ops.rs
index 13dd188..e46765e 100644
--- a/core/runtime/tests/ops.rs
+++ b/core/runtime/tests/ops.rs
@@ -9,7 +9,6 @@ use crate::*;
 use anyhow::bail;
 use anyhow::Error;
 use futures::Future;
-use log::debug;
 use pretty_assertions::assert_eq;
 use std::cell::RefCell;
 use std::rc::Rc;
@@ -515,7 +514,6 @@ await op_void_async_deferred();
     biased;
 
     maybe_result = &mut rx => {
-      debug!("received module evaluate {:#?}", maybe_result);
       maybe_result
     }
 
diff --git a/ops/op2/test_cases/sync/cppgc_resource.out b/ops/op2/test_cases/sync/cppgc_resource.out
index 5c99b71..29916a4 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.out
+++ b/ops/op2/test_cases/sync/cppgc_resource.out
@@ -234,3 +234,110 @@ impl op_make_cppgc_object {
         Wrap
     }
 }
+
+#[allow(non_camel_case_types)]
+struct op_make_cppgc_object_async {
+    _unconstructable: ::std::marker::PhantomData<()>,
+}
+impl ::deno_core::_ops::Op for op_make_cppgc_object_async {
+    const NAME: &'static str = stringify!(op_make_cppgc_object_async);
+    const DECL: ::deno_core::_ops::OpDecl = ::deno_core::_ops::OpDecl::new_internal_op2(
+        ::deno_core::__op_name_fast!(op_make_cppgc_object_async),
+        true,
+        false,
+        1usize as u8,
+        Self::v8_fn_ptr as _,
+        Self::v8_fn_ptr_metrics as _,
+        None,
+        None,
+        ::deno_core::OpMetadata {
+            ..::deno_core::OpMetadata::default()
+        },
+    );
+}
+impl op_make_cppgc_object_async {
+    pub const fn name() -> &'static str {
+        stringify!(op_make_cppgc_object_async)
+    }
+    #[deprecated(note = "Use the const op::DECL instead")]
+    pub const fn decl() -> deno_core::_ops::OpDecl {
+        <Self as deno_core::_ops::Op>::DECL
+    }
+    #[inline(always)]
+    fn slow_function_impl(info: *const deno_core::v8::FunctionCallbackInfo) -> usize {
+        #[cfg(debug_assertions)]
+        let _reentrancy_check_guard = deno_core::_ops::reentrancy_check(
+            &<Self as deno_core::_ops::Op>::DECL,
+        );
+        let mut scope = unsafe { deno_core::v8::CallbackScope::new(&*info) };
+        let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe {
+            &*info
+        });
+        let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+            &*info
+        });
+        let opctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        let result = { Self::call() };
+        let promise_id = deno_core::_ops::to_i32_option(&args.get(0))
+            .unwrap_or_default();
+        if let Some(result) = deno_core::_ops::map_async_op_infallible(
+            opctx,
+            false,
+            false,
+            promise_id,
+            result,
+            |scope, result| {
+                Ok(
+                    deno_core::_ops::RustToV8NoScope::to_v8(
+                        deno_core::cppgc::make_cppgc_object(scope, result),
+                    ),
+                )
+            },
+        ) {
+            rv.set(
+                deno_core::_ops::RustToV8NoScope::to_v8(
+                    deno_core::v8::Local::<
+                        deno_core::v8::Value,
+                    >::from(deno_core::cppgc::make_cppgc_object(&mut scope, result)),
+                ),
+            );
+            return 0;
+        }
+        return 2;
+    }
+    extern "C" fn v8_fn_ptr(info: *const deno_core::v8::FunctionCallbackInfo) {
+        Self::slow_function_impl(info);
+    }
+    extern "C" fn v8_fn_ptr_metrics(info: *const deno_core::v8::FunctionCallbackInfo) {
+        let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe {
+            &*info
+        });
+        let opctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        deno_core::_ops::dispatch_metrics_async(
+            &opctx,
+            deno_core::_ops::OpMetricsEvent::Dispatched,
+        );
+        let res = Self::slow_function_impl(info);
+        if res == 0 {
+            deno_core::_ops::dispatch_metrics_async(
+                &opctx,
+                deno_core::_ops::OpMetricsEvent::Completed,
+            );
+        } else if res == 1 {
+            deno_core::_ops::dispatch_metrics_async(
+                &opctx,
+                deno_core::_ops::OpMetricsEvent::Error,
+            );
+        }
+    }
+    #[inline(always)]
+    async fn call() -> Wrap {
+        Wrap
+    }
+}
diff --git a/ops/op2/test_cases/sync/cppgc_resource.rs b/ops/op2/test_cases/sync/cppgc_resource.rs
index 2c77500..939bc7e 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.rs
+++ b/ops/op2/test_cases/sync/cppgc_resource.rs
@@ -12,3 +12,9 @@ struct Wrap;
 fn op_make_cppgc_object() -> Wrap {
     Wrap
 }
+
+#[op2(async)]
+#[cppgc]
+async fn op_make_cppgc_object_async() -> Wrap {
+    Wrap
+}
diff --git a/testing/checkin/runner/mod.rs b/testing/checkin/runner/mod.rs
index b7374a4..075d3a0 100644
--- a/testing/checkin/runner/mod.rs
+++ b/testing/checkin/runner/mod.rs
@@ -52,6 +52,8 @@ deno_core::extension!(
     ops_async::op_async_barrier_create,
     ops_async::op_async_barrier_await,
     ops_async::op_async_spin_on_state,
+    ops_async::op_async_make_cppgc_resource,
+    ops_async::op_async_get_cppgc_resource,
     ops_error::op_async_throw_error_eager,
     ops_error::op_async_throw_error_lazy,
     ops_error::op_async_throw_error_deferred,
diff --git a/testing/checkin/runner/ops_async.rs b/testing/checkin/runner/ops_async.rs
index 4a6cd54..b8bda5b 100644
--- a/testing/checkin/runner/ops_async.rs
+++ b/testing/checkin/runner/ops_async.rs
@@ -46,3 +46,21 @@ pub async fn op_async_spin_on_state(state: Rc<RefCell<OpState>>) {
   })
   .await
 }
+
+pub struct TestResource {
+  value: u32,
+}
+
+#[op2(async)]
+#[cppgc]
+pub async fn op_async_make_cppgc_resource() -> TestResource {
+  TestResource { value: 42 }
+}
+
+#[op2(async)]
+#[smi]
+pub async fn op_async_get_cppgc_resource(
+  #[cppgc] resource: &TestResource,
+) -> u32 {
+  resource.value
+}
diff --git a/testing/unit/resource_test.ts b/testing/unit/resource_test.ts
index b208b25..5b52a7d 100644
--- a/testing/unit/resource_test.ts
+++ b/testing/unit/resource_test.ts
@@ -1,7 +1,12 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 import { assert, assertArrayEquals, assertEquals, test } from "checkin:testing";
 
-const { op_pipe_create, op_file_open } = Deno.core.ops;
+const {
+  op_pipe_create,
+  op_file_open,
+  op_async_make_cppgc_resource,
+  op_async_get_cppgc_resource,
+} = Deno.core.ops;
 
 test(async function testPipe() {
   const [p1, p2] = op_pipe_create();
@@ -52,3 +57,8 @@ test(async function testFileIsNotTerminal() {
   const file = await op_file_open("./README.md");
   assert(!Deno.core.isTerminal(file));
 });
+
+test(async function testCppgcAsync() {
+  const resource = await op_async_make_cppgc_resource();
+  assertEquals(await op_async_get_cppgc_resource(resource), 42);
+});