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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ pub use crate::modules::ResolutionKind;
pub use crate::modules::StaticModuleLoader;
pub use crate::modules::ValidateImportAttributesCb;
pub use crate::normalize_path::normalize_path;
pub use crate::ops::OpCtx;
pub use crate::ops::OpId;
pub use crate::ops::OpMetadata;
pub use crate::ops::OpState;
Expand Down Expand Up @@ -171,6 +172,7 @@ pub mod _ops {
pub use super::ops_metrics::dispatch_metrics_fast;
pub use super::ops_metrics::dispatch_metrics_slow;
pub use super::ops_metrics::OpMetricsEvent;
pub use super::runtime::bindings::register_op_method;
pub use super::runtime::ops::*;
pub use super::runtime::ops_rust_to_v8::*;
pub use super::runtime::V8_WRAPPER_OBJECT_INDEX;
Expand Down
14 changes: 14 additions & 0 deletions core/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,20 @@ impl OpCtx {
}
}

pub fn clone_for_method(&self, decl: OpDecl) -> Self {
// id and metrics_fn are not used in method ops
Self::new(
0,
self.isolate,
self.op_driver.clone(),
decl,
self.state.clone(),
self.runtime_state.clone(),
self.get_error_class_fn,
None,
)
}

#[inline(always)]
pub const fn decl(&self) -> &OpDecl {
&self.decl
Expand Down
11 changes: 11 additions & 0 deletions core/runtime/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

scope: &mut v8::HandleScope,
op_ctx: OpCtx,
obj: v8::Local<v8::Object>,
) {
let key = op_ctx.decl.name_fast.v8_string(scope);
let op_fn = op_ctx_function(scope, Box::leak(Box::new(op_ctx)));

obj.set(scope, key.into(), op_fn.into()).unwrap();
}

fn op_ctx_function<'s>(
scope: &mut v8::HandleScope<'s>,
op_ctx: &OpCtx,
Expand Down
2 changes: 0 additions & 2 deletions core/runtime/jsruntime.rs
Original file line number Diff line number Diff line change
Expand 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.

}

pub fn prepare_for_snapshot(mut self) -> v8::OwnedIsolate {
Expand Down
29 changes: 29 additions & 0 deletions ops/op2/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ pub(crate) struct MacroConfig {
pub async_deferred: bool,
/// Marks an op as re-entrant (can safely call other ops).
pub reentrant: bool,
/// Marks an op as a method on a wrapped object.
pub method: Option<String>,
}

impl MacroConfig {
Expand Down Expand Up @@ -85,6 +87,17 @@ impl MacroConfig {
config.async_deferred = true;
} else if flag == "reentrant" {
config.reentrant = true;
} else if flag.starts_with("method(") {
let tokens =
syn::parse_str::<TokenTree>(&flag[6..])?.into_token_stream();
config.method = std::panic::catch_unwind(|| {
rules!(tokens => {
( ( $s:ty ) ) => {
Some(s.into_token_stream().to_string())
}
})
})
.map_err(|_| Op2Error::PatternMatchFailed("attribute", flag))?;
} else {
return Err(Op2Error::InvalidAttribute(flag));
}
Expand Down Expand Up @@ -226,5 +239,21 @@ mod tests {
..Default::default()
},
);

test_parse(
"(method(A))",
MacroConfig {
method: Some("A".to_owned()),
..Default::default()
},
);
test_parse(
"(fast, method(T))",
MacroConfig {
method: Some("T".to_owned()),
fast: true,
..Default::default()
},
);
}
}
8 changes: 8 additions & 0 deletions ops/op2/dispatch_async.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use super::dispatch_slow::with_opctx;
use super::dispatch_slow::with_opstate;
use super::dispatch_slow::with_retval;
use super::dispatch_slow::with_scope;
use super::dispatch_slow::with_self;
use super::generator_state::gs_quote;
use super::generator_state::GeneratorState;
use super::signature::ParsedSignature;
Expand Down Expand Up @@ -136,6 +137,12 @@ pub(crate) fn generate_dispatch_async(
quote!()
};

let with_self = if generator_state.needs_self {
with_self(generator_state)
} else {
quote!()
};

Ok(
gs_quote!(generator_state(info, slow_function, slow_function_metrics, opctx) => {
#[inline(always)]
Expand All @@ -148,6 +155,7 @@ pub(crate) fn generate_dispatch_async(
#with_args
#with_opctx
#with_opstate
#with_self

#output
}
Expand Down
26 changes: 24 additions & 2 deletions ops/op2/dispatch_fast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

})
} else {
quote!()
};

let name = &generator_state.name;
let call = if generator_state.needs_self {
quote!(self_. #name)
} else {
quote!(Self:: #name)
};

let with_opctx = if generator_state.needs_fast_opctx {
generator_state.needs_fast_api_callback_options = true;
gs_quote!(generator_state(opctx, fast_api_callback_options) => {
Expand Down Expand Up @@ -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.

this: deno_core::v8::Local<deno_core::v8::Object>,
#( #fastcall_names: #fastcall_types, )*
) -> #output_type {
#[cfg(debug_assertions)]
Expand All @@ -512,9 +527,11 @@ pub(crate) fn generate_dispatch_fast(
#with_fast_api_callback_options
#with_opctx
#with_js_runtime_state
#with_self

let #result = {
#(#call_args)*
Self::call(#(#call_names),*)
#call (#(#call_names),*)
};
#handle_error
#handle_result
Expand Down Expand Up @@ -624,6 +641,10 @@ fn map_v8_fastcall_arg_to_arg(
*needs_js_runtime_state = true;
quote!(let #arg_ident = &#js_runtime_state;)
}
Arg::Ref(RefType::Ref, Special::OpCtx) => {
*needs_opctx = true;
quote!(let #arg_ident = #opctx;)
}
Arg::State(RefType::Ref, state) => {
*needs_opctx = true;
let state =
Expand Down Expand Up @@ -783,6 +804,7 @@ fn map_arg_to_v8_fastcall_type(
| Arg::Ref(_, Special::OpState)
| Arg::Rc(Special::JsRuntimeState)
| Arg::Ref(RefType::Ref, Special::JsRuntimeState)
| Arg::Ref(RefType::Ref, Special::OpCtx)
| Arg::State(..)
| Arg::Special(Special::Isolate)
| Arg::OptionState(..)
Expand Down
28 changes: 27 additions & 1 deletion ops/op2/dispatch_slow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ pub(crate) fn generate_dispatch_slow(
quote!()
};

let with_self = if generator_state.needs_self {
with_self(generator_state)
} else {
quote!()
};

Ok(
gs_quote!(generator_state(opctx, info, slow_function, slow_function_metrics) => {
#[inline(always)]
Expand All @@ -149,6 +155,7 @@ pub(crate) fn generate_dispatch_slow(
#with_isolate
#with_opstate
#with_js_runtime_state
#with_self

#output;
return 0;
Expand Down Expand Up @@ -236,6 +243,13 @@ pub(crate) fn with_js_runtime_state(
)
}

pub(crate) fn with_self(generator_state: &mut GeneratorState) -> TokenStream {
generator_state.needs_opctx = true;
gs_quote!(generator_state(fn_args, self_ty) =>
(let self_: &#self_ty = unsafe { deno_core::cppgc::try_unwrap_cppgc_object(#fn_args.this().into()).unwrap() };)
littledivy marked this conversation as resolved.
Show resolved Hide resolved
)
}

pub fn extract_arg(
generator_state: &mut GeneratorState,
index: usize,
Expand Down Expand Up @@ -430,6 +444,10 @@ pub fn from_arg(
*needs_js_runtime_state = true;
quote!(let #arg_ident = &#js_runtime_state;)
}
Arg::Ref(RefType::Ref, Special::OpCtx) => {
*needs_opctx = true;
quote!(let #arg_ident = #opctx;)
}
Arg::State(RefType::Ref, state) => {
*needs_opstate = true;
let state =
Expand Down Expand Up @@ -679,7 +697,15 @@ pub fn call(generator_state: &mut GeneratorState) -> TokenStream {
for arg in &generator_state.args {
tokens.extend(quote!( #arg , ));
}
quote!(Self::call( #tokens ))

let name = &generator_state.name;
let call_ = if generator_state.needs_self {
quote!(self_. #name)
} else {
quote!(Self:: #name)
};

quote!(#call_ ( #tokens ))
}

pub fn return_value(
Expand Down
4 changes: 4 additions & 0 deletions ops/op2/generator_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
use proc_macro2::Ident;

pub struct GeneratorState {
pub name: Ident,
/// Identifiers for each of the arguments of the original function
pub args: Vec<Ident>,
/// The result of the `call` function
Expand Down Expand Up @@ -33,6 +34,8 @@ pub struct GeneratorState {
pub fast_function_metrics: Ident,
/// The async function promise ID argument
pub promise_id: Ident,
/// Type of the self argument
pub self_ty: Ident,

pub needs_args: bool,
pub needs_retval: bool,
Expand All @@ -44,6 +47,7 @@ pub struct GeneratorState {
pub needs_fast_opctx: bool,
pub needs_fast_api_callback_options: bool,
pub needs_fast_js_runtime_state: bool,
pub needs_self: bool,
}

/// Quotes a set of generator_state fields, along with variables captured from
Expand Down
Loading
Loading