feat: op2 codegen for object methods #682

littledivy · 2024-03-30T13:40:54Z

Introducing op2++ (a.k.a op2 for object methods).

op2++ is to op2 as C++ is to C.

This patch introduces a small extension on top of the existing op2 codegen infrastructure to enable writing cppgc object methods in Rust:

pub struct City {
  name: String,
}

impl City {
  #[op2(method(City))]
  #[string]
  fn get_name(&self) -> String {
    self.name.clone()
  }

  #[op2(fast, method(City))]
  fn print_name(&self) {
    println!("{}", self.name);
  }
}

#[op2]
pub fn op_city_new<'a>(
  scope: &mut v8::HandleScope<'a>,
  ctx: &OpCtx,
  #[string] name: String,
) -> v8::Local<'a, v8::Object> {
  let obj = deno_core::cppgc::make_cppgc_object(scope, City { name });
  City::register_get_name(scope, ctx, obj);
  City::register_print_name(scope, ctx, obj);
  obj
}

import { op_city_new } from "ext:core/ops";

const city = op_city_new("Pune");
city.print_name();

const name = city.get_name();

Notable changes:

Added &OpCtx as argument, required for registration of methods.
New attribute method(Type) where Type is the type of the reciever self. Due to the independent nature of op2 codegen we need to know the concrete type.
Generate registration methods Type::register_op that can be called to register the method onto a Cppgc object.

Future work could make these constructors transparent by processing impls.

codecov-commenter · 2024-03-30T13:51:14Z

Codecov Report

Attention: Patch coverage is 95.12195% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 81.23%. Comparing base (79e73e5) to head (5a0fdb6).

Files	Patch %	Lines
testing/checkin/runner/ops.rs	75.00%	6 Missing ⚠️
ops/op2/dispatch_fast.rs	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #682      +/-   ##
==========================================
+ Coverage   81.14%   81.23%   +0.09%     
==========================================
  Files          96       96              
  Lines       24347    24493     +146     
==========================================
+ Hits        19756    19897     +141     
- Misses       4591     4596       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mmastrac

Let's do a small change to break the this change out and then work through this.

mmastrac · 2024-04-02T14:52:49Z

ops/op2/dispatch_fast.rs

@@ -503,7 +518,7 @@ pub(crate) fn generate_dispatch_fast(

    #[allow(clippy::too_many_arguments)]
    extern "C" fn #fast_function(
-      _: deno_core::v8::Local<deno_core::v8::Object>,


Let's split this one line change into its own PR + the test changes to reduce the PR size.

it will be a warning (unused argument) if landed seperately

No worries -- just add allow(unused) for it. We can remove that at a later date.

mmastrac · 2024-04-02T22:31:27Z

core/runtime/bindings.rs

@@ -323,6 +323,17 @@ pub(crate) fn initialize_deno_core_ops_bindings<'s>(
  }
 }

+pub fn register_op_method(


We should try to create these once when the JsRuntime is created and then look them up.

mmastrac · 2024-04-02T22:31:50Z

core/runtime/jsruntime.rs

@@ -183,8 +183,6 @@ impl InnerIsolateState {
    unsafe {
      ManuallyDrop::take(&mut self.main_realm).0.destroy();
    }
-
-    debug_assert_eq!(Rc::strong_count(&self.state), 1);


This is definitely a smell here -- we're leaking OpCtxs.

mmastrac · 2024-04-02T22:32:12Z

ops/op2/dispatch_fast.rs

@@ -439,6 +439,21 @@ pub(crate) fn generate_dispatch_fast(
    quote!()
  };

+  let with_self = if generator_state.needs_self {
+    gs_quote!(generator_state(self_ty) => {
+      let self_: &#self_ty = deno_core::cppgc::try_unwrap_cppgc_object(this.into()).unwrap();


👍 awesome

mmastrac · 2024-04-02T22:33:29Z

ops/op2/mod.rs

+  };
+
+  if *needs_self {
+    let register = format_ident!("register_{name}");


This shouldn't be necessary -- this code is effectively the same as if it were on the OpDecl itself.

ops/op2/dispatch_slow.rs

mmastrac · 2024-04-02T22:41:57Z

ops/op2/mod.rs

@@ -122,13 +124,14 @@ fn generate_op2(
    zip(signature.args.iter(), &func.sig.inputs).collect::<Vec<_>>();

  let mut args = vec![];
-  let mut needs_args = false;
+  let mut needs_args = config.method.is_some();


Aren't we using the receiver for self? This shouldn't change, right?

mmastrac · 2024-04-02T22:42:29Z

ops/op2/signature.rs

@@ -1345,6 +1349,7 @@ fn parse_type_path(
      }
      ( OpState ) => Ok(CBare(TSpecial(Special::OpState))),
      ( JsRuntimeState ) => Ok(CBare(TSpecial(Special::JsRuntimeState))),
+      ( OpCtx ) => Ok(CBare(TSpecial(Special::OpCtx))),


I don't think any of this OpCtx stuff will be necessary in the end

mmastrac · 2024-04-02T23:04:15Z

Ok, looked through this PR a few times and I think I know the right plan here. Ignore my previous comment about a this param change being separate.Let's do it in this order:

Split out just enough of this PR to support &self params in op2. Ignore the registration part entirely and just manage the cppgc unwrapping functionality. Don't generate any registration methods at all, and let's add tests that bare, &self-receiver methods correctly work w/cppgc objects. We should not require any sort of special annotation for these -- just specifying a self receiver should be enough to detect that this is an impl method, and it will fail to compile if there's no surrounding impl.

eg, this should work:

impl Foo {
  #[op2(method(Foo))]
  pub fn bar(&self) {}
}

We don't currently support this style of op because we're using a struct, and structs can't be nested in impl but if we switch to using a const, we can make something that works for all cases (standalone global op, associated method, self-receiver instance method):

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=407c2a059815c64a00903280a8d69698

Add a registry of op2 methods to generated v8::Function to JsRuntime. If we can ensure that each op has a unique &static OpDecl, this can be a simple map. Track the final generated function for each extension op2 OpDecl in a map, and also ensure that it survives snapshotting. If extension middleware modifies an op, we should be able to retrieve the final op function, even if it's been disabled or nop'd.
Create a macro to attach a set of instance methods to an impl. Maybe something like members!(get_name, print_name, async_method) . This will generate a slice of static OpDecls that can be used for auto-registration.
Add a &[&'static OpDecl] parameter to make_cppgc_object that defines the members to attach to the created object. In make_cppgc_object you should be able to access that registry of op2 methods, retrieve v8::Local<v8::Function> for each of those decls, and then perform the object-set operation. At a later date, we can pre-generate v8::ObjectTemplates for each cppgc object to make this cheaper.

Switch op declaration from a `impl #name` to a `const fn #name`. Based on Matt's suggestion in denoland#682 [Playground link](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=2d7152d548b4a466779c401551b21b05) This patch wraps `const` around the existing `impl` codegen to unblock work on op2++

Switch op declaration from a `impl #name` to a `const fn #name`. This form for declaration can be used inside `impl` blocks. ```rust struct Foo {} impl Foo { #[op2(fast)] pub fn bar(#[state] state: &mut OpState) {} } // const DECL: OpDecl = Foo::bar(); ``` Based on Matt's suggestion in #682 [Playground link](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=2d7152d548b4a466779c401551b21b05) This patch wraps `const` around the existing `impl` codegen to unblock work on op2++

Refactor out codegen for `&self` from #682 ```rust struct State { name: &'static str, } impl State { #[op2(fast, method(State)] fn print(&self) { println!("{}", self.name); } } const STATE_DECL: [OpDecl; 1] = [ State::print() ]; ```

littledivy added 6 commits March 30, 2024 13:06

feat: op2 codegen for object methods

c5a9567

preserve method name

893abee

async and fast ops support

ce39759

works!

3070426

fmt

fef7482

lint

5a0fdb6

littledivy requested review from mmastrac and bartlomieju March 31, 2024 07:36

mmastrac reviewed Apr 2, 2024

View reviewed changes

ops/op2/dispatch_slow.rs Show resolved Hide resolved

mmastrac reviewed Apr 2, 2024

View reviewed changes

littledivy mentioned this pull request Apr 6, 2024

Use const fn for op declaration #685

Merged

littledivy mentioned this pull request Apr 7, 2024

op2++: codegen for &self parameter #686

Merged

petamoriken mentioned this pull request May 26, 2024

feat(ext/geometry): Geometry Interfaces Module Level 1 denoland/deno#22054

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: op2 codegen for object methods #682

feat: op2 codegen for object methods #682

littledivy commented Mar 30, 2024

codecov-commenter commented Mar 30, 2024

mmastrac left a comment

mmastrac Apr 2, 2024

littledivy Apr 2, 2024

mmastrac Apr 2, 2024 •

edited

mmastrac Apr 2, 2024

mmastrac Apr 2, 2024

mmastrac Apr 2, 2024

mmastrac Apr 2, 2024

mmastrac Apr 2, 2024

mmastrac Apr 2, 2024

mmastrac commented Apr 2, 2024 •

edited

feat: op2 codegen for object methods #682

Are you sure you want to change the base?

feat: op2 codegen for object methods #682

Conversation

littledivy commented Mar 30, 2024

codecov-commenter commented Mar 30, 2024

Codecov Report

mmastrac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmastrac Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmastrac commented Apr 2, 2024 • edited

mmastrac Apr 2, 2024 •

edited

mmastrac commented Apr 2, 2024 •

edited