Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: op2 codegen for object methods #682

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

littledivy
Copy link
Member

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
Copy link

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.

Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mmastrac mmastrac Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

pub fn register_op_method(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

debug_assert_eq!(Rc::strong_count(&self.state), 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 awesome

};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mmastrac
Copy link
Contributor

mmastrac commented Apr 2, 2024

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:

  1. 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

  1. 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.

  2. 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.

  3. 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.

littledivy added a commit to littledivy/deno_core that referenced this pull request Apr 6, 2024
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++
littledivy added a commit that referenced this pull request Apr 6, 2024
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++
littledivy added a commit that referenced this pull request Apr 8, 2024
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()
];
```
@gearonix
Copy link

Awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants