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

op2 object wraps #805

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

op2 object wraps #805

wants to merge 19 commits into from

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Jul 4, 2024

Object wrap for Cppgc-backed objects

#[op2] will generate the glue code declarations for impl blocks to create JS objects in Rust using the op2 infra.

deno_core::extension!(
   // ...
  objects = [MyObject],
)

Currently supported bindings:

  • constructor
  • methods
  • static methods

Planned support:

  • getters
  • setters

Future:

  • webidl validation

Example:

// dompoint.rs
pub struct DOMPoint {
  x: f64,
  y: f64,
  z: f64,
  w: f64,
}

impl GarbageCollected for DOMPoint {}

#[op2]
impl DOMPoint {
  #[constructor]
  #[cppgc]
  fn new(
    x: Option<f64>,
    y: Option<f64>,
    z: Option<f64>,
    w: Option<f64>,
  ) -> DOMPoint {
    DOMPoint {
      x: x.unwrap_or(0.0),
      y: y.unwrap_or(0.0),
      z: z.unwrap_or(0.0),
      w: w.unwrap_or(0.0),
    }
  }

  #[static_method]
  #[cppgc]
  fn from_point(
    scope: &mut v8::HandleScope,
    other: v8::Local<v8::Object>,
  ) -> Result<DOMPoint, AnyError> {
    fn get(
      scope: &mut v8::HandleScope,
      other: v8::Local<v8::Object>,
      key: &str,
    ) -> Option<f64> {
      let key = v8::String::new(scope, key).unwrap();
      other
        .get(scope, key.into())
        .map(|x| x.to_number(scope).unwrap().value())
    }

    Ok(DOMPoint {
      x: get(scope, other, "x").unwrap_or(0.0),
      y: get(scope, other, "y").unwrap_or(0.0),
      z: get(scope, other, "z").unwrap_or(0.0),
      w: get(scope, other, "w").unwrap_or(0.0),
    })
  }

  #[fast]
  fn x(&self) -> f64 {
    self.x
  }
}
import { DOMPoint } from "ext:core/ops";

const p = new DOMPoint(200, 300);
p.x(); // 200.0

core/cppgc.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 18 lines in your changes missing coverage. Please review.

Project coverage is 81.57%. Comparing base (0c7f83e) to head (b2cd2ea).
Report is 128 commits behind head on main.

Files with missing lines Patch % Lines
testing/checkin/runner/ops.rs 81.81% 8 Missing ⚠️
ops/op2/dispatch_fast.rs 37.50% 5 Missing ⚠️
ops/op2/object_wrap.rs 96.87% 2 Missing ⚠️
core/extensions.rs 90.90% 1 Missing ⚠️
ops/op2/config.rs 90.90% 1 Missing ⚠️
ops/op2/dispatch_slow.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #805      +/-   ##
==========================================
+ Coverage   81.43%   81.57%   +0.14%     
==========================================
  Files          97       99       +2     
  Lines       23877    25092    +1215     
==========================================
+ Hits        19445    20470    +1025     
- Misses       4432     4622     +190     

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

Comment on lines +41 to +49
// To initialize object wraps correctly, we store the function
// template in OpState with `T`'s TypeId as the key when binding
// because it'll be pretty annoying to propogate `T` generic everywhere.
//
// Here we try to retrive a function template for `T`, falling back to
// the default cppgc template.
let id = TypeId::of::<T>();
let obj = if let Some(templ) =
opstate.try_borrow_untyped::<v8::Global<v8::FunctionTemplate>>(id)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good solution. @devsnek thoughts?

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Awesome work! Let's get @nathanwhit or @devsnek to give it another pass. This infra is great and am looking forward to rewrites of some of Deno APIs to use native code.

CC @petamoriken this PR will make it possible to land denoland/deno#22054.

core/extension_set.rs Outdated Show resolved Hide resolved
core/extension_set.rs Show resolved Hide resolved
core/extension_set.rs Outdated Show resolved Hide resolved
core/extension_set.rs Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
core/runtime/bindings.rs Show resolved Hide resolved
core/runtime/bindings.rs Show resolved Hide resolved
// ```rust
// deno_core::extension!(
// ...,
// objects = [MyObject],
Copy link
Member

Choose a reason for hiding this comment

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

Should we bike-shed on the name? prototypes?

// ```
//
// ```js
// import { MyObject } from "ext:core/ops";
Copy link
Member

Choose a reason for hiding this comment

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

Feels somewhat odd that we import it from ext:core/ops, fine for the first pass though 👍

Comment on lines +67 to +85
test(function testDomPoint() {
const p1 = new DOMPoint(100, 100);
const p2 = new DOMPoint();
const p3 = DOMPoint.from_point({ x: 200 });
const p4 = DOMPoint.from_point({ x: 0, y: 100, z: 99.9, w: 100 });
assertEquals(p1.x(), 100);
assertEquals(p2.x(), 0);
assertEquals(p3.x(), 200);
assertEquals(p4.x(), 0);

let caught;
try {
// @ts-expect-error bad arg test
new DOMPoint("bad");
} catch (e) {
caught = e;
}
assert(caught);
});
Copy link
Member

Choose a reason for hiding this comment

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

This is incredible

self.name.len() as u32
#[static_method]
#[cppgc]
fn from_point(
Copy link
Member

Choose a reason for hiding this comment

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

should we have a #[rename("fromPoint")] attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if well-known symbols could also be supported

let id = op_method_ctx.id;
op_state
.borrow_mut()
.put_untyped(id, v8::Global::new(scope, tmpl));
Copy link
Member

Choose a reason for hiding this comment

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

do these need to be registered as snapshot data?

@ry
Copy link
Member

ry commented Jul 6, 2024

Have you done any performance benchmarks?

@littledivy
Copy link
Member Author

$ target/release/dcore bench.js
JS   x(): 87ms
Rust x(): 285ms
JS   create: 904ms
Rust create: 1706ms
Benchmark
const { DOMPoint } = Deno.core.ops;

function bench(name, fn, iterations = 1e7) {
  var start = Date.now();
  for (var i = 0; i < iterations; i++) {
    fn();
  }
  var end = Date.now();
  console.log(name + ': ' + (end - start) + 'ms');
}

class DOMPointJS {
  #x;
  #y;
  #z;
  #w;

  constructor(x, y, z, w) {
    this.#x = x || 0;
    this.#y = y || 0;
    this.#z = z || 0;
    this.#w = w || 1;
  }

  x() {
    return this.#x;
  }
}

const p = new DOMPointJS(100, 200);
function getXJS() {
   return p.x()
}

const p2 = new DOMPoint(100, 200);
function getXRust() {
   return p2.x();
}

bench('JS   x()', getXJS);
bench('Rust x()', getXRust);

function createPointJS() {
  return new DOMPointJS(100, 200);
}

function createPointRust() {
   return new DOMPoint(100, 200);
}

bench('JS   create', createPointJS);
bench('Rust create', createPointRust);

So...not great atm, working on it. Object::wrap and Object::unwrap can take Isolate instead of HandleScopes so we can avoid creating and dropping scope in most cases - which is 30% of the overhead.

Comment on lines +125 to +128
#[fast]
fn x(&self) -> f64 {
self.x
}
Copy link
Contributor

@petamoriken petamoriken Jul 6, 2024

Choose a reason for hiding this comment

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

Can we also define accessor properties?

Copy link
Member Author

@littledivy littledivy Jul 6, 2024

Choose a reason for hiding this comment

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

not in this PR but planned. It's a bit more work because accessors have different function signatures than normal ops

@littledivy
Copy link
Member Author

Update benchmarks:

JS   x(): 84ms
Rust x(): 191ms
JS   create: 953ms
Rust create: 1810ms

Copy link
Member

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

LGTM

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.

7 participants