From a181ceb0e3791c842db6e8e6f528cf9ce320642a Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Sat, 24 Jun 2023 23:30:04 +0200 Subject: [PATCH] refactor(ops): op2 supports Result in slow call path (#19602) --- core/lib.rs | 5 - core/runtime/ops.rs | 72 ++++++++++- ops/op2/dispatch_fast.rs | 5 + ops/op2/dispatch_slow.rs | 119 ++++++++++++++++--- ops/op2/generator_state.rs | 4 + ops/op2/mod.rs | 6 +- ops/op2/test_cases/sync/add_options.out | 16 ++- ops/op2/test_cases/sync/result_primitive.out | 50 ++++++++ ops/op2/test_cases/sync/result_primitive.rs | 4 + ops/op2/test_cases/sync/result_void.out | 45 +++++++ ops/op2/test_cases/sync/result_void.rs | 4 + 11 files changed, 295 insertions(+), 35 deletions(-) create mode 100644 ops/op2/test_cases/sync/result_primitive.out create mode 100644 ops/op2/test_cases/sync/result_primitive.rs create mode 100644 ops/op2/test_cases/sync/result_void.out create mode 100644 ops/op2/test_cases/sync/result_void.rs diff --git a/core/lib.rs b/core/lib.rs index 9a960e93ab..1042bf55c6 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -147,11 +147,6 @@ pub mod _ops { pub use super::runtime::V8_WRAPPER_TYPE_INDEX; } -pub(crate) mod deno_core { - pub(crate) use crate::_ops; - pub(crate) use crate::v8; -} - // TODO(mmastrac): Temporary while we move code around pub mod snapshot_util { pub use crate::runtime::create_snapshot; diff --git a/core/runtime/ops.rs b/core/runtime/ops.rs index fb71de8cbb..85708b24be 100644 --- a/core/runtime/ops.rs +++ b/core/runtime/ops.rs @@ -208,12 +208,25 @@ pub fn to_i64(number: &v8::Value) -> i32 { #[cfg(test)] mod tests { + use crate::error::generic_error; + use crate::error::AnyError; + use crate::error::JsError; use crate::FastString; use crate::JsRuntime; use crate::RuntimeOptions; use deno_ops::op2; - crate::extension!(testing, ops = [op_test_add, op_test_add_option]); + crate::extension!( + testing, + ops = [ + op_test_add, + op_test_add_option, + op_test_result_void_ok, + op_test_result_void_err, + op_test_result_primitive_ok, + op_test_result_primitive_err + ] + ); /// Run a test for a single op. fn run_test( @@ -281,4 +294,61 @@ mod tests { ); Ok(()) } + + #[op2(core)] + pub fn op_test_result_void_err() -> Result<(), AnyError> { + Err(generic_error("failed!!!")) + } + + #[op2(core)] + pub fn op_test_result_void_ok() -> Result<(), AnyError> { + Ok(()) + } + + #[tokio::test] + pub async fn test_op_result_void() -> Result<(), Box> { + run_test( + "op_test_result_void_err", + "op_test_result_void_err()", + |value, _scope| { + let js_error = value.err().unwrap().downcast::().unwrap(); + assert_eq!(js_error.message, Some("failed!!!".to_owned())); + }, + ); + run_test( + "op_test_result_void_ok", + "op_test_result_void_ok()", + |value, _scope| assert!(value.unwrap().is_null_or_undefined()), + ); + Ok(()) + } + + #[op2(core)] + pub fn op_test_result_primitive_err() -> Result { + Err(generic_error("failed!!!")) + } + + #[op2(core)] + pub fn op_test_result_primitive_ok() -> Result { + Ok(123) + } + + #[tokio::test] + pub async fn test_op_result_primitive( + ) -> Result<(), Box> { + run_test( + "op_test_result_primitive_err", + "op_test_result_primitive_err()", + |value, _scope| { + let js_error = value.err().unwrap().downcast::().unwrap(); + assert_eq!(js_error.message, Some("failed!!!".to_owned())); + }, + ); + run_test( + "op_test_result_primitive_ok", + "op_test_result_primitive_ok()", + |value, scope| assert_eq!(value.unwrap().int32_value(scope), Some(123)), + ); + Ok(()) + } } diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs index 79b8d141b4..94140dbf6f 100644 --- a/ops/op2/dispatch_fast.rs +++ b/ops/op2/dispatch_fast.rs @@ -95,6 +95,11 @@ pub fn generate_dispatch_fast( generator_state: &mut GeneratorState, signature: &ParsedSignature, ) -> Result, V8MappingError> { + // Result not fast-call compatible (yet) + if matches!(signature.ret_val, RetVal::Result(..)) { + return Ok(None); + } + let mut inputs = vec![]; for arg in &signature.args { let fv = match arg { diff --git a/ops/op2/dispatch_slow.rs b/ops/op2/dispatch_slow.rs index dd47b2017b..f54a28f1c0 100644 --- a/ops/op2/dispatch_slow.rs +++ b/ops/op2/dispatch_slow.rs @@ -18,37 +18,34 @@ pub fn generate_dispatch_slow( output.extend(extract_arg(generator_state, index)?); output.extend(from_arg(generator_state, index, arg)?); } - output.extend(call(generator_state)); - output.extend(return_value(generator_state, &signature.ret_val)); - - let GeneratorState { - deno_core, - scope, - fn_args, - retval, - info, - slow_function, - .. - } = &generator_state; + output.extend(call(generator_state)?); + output.extend(return_value(generator_state, &signature.ret_val)?); let with_scope = if generator_state.needs_scope { - quote!(let #scope = &mut unsafe { #deno_core::v8::CallbackScope::new(&*#info) };) + with_scope(generator_state) } else { quote!() }; let with_retval = if generator_state.needs_retval { - quote!(let mut #retval = #deno_core::v8::ReturnValue::from_function_callback_info(unsafe { &*#info });) + with_retval(generator_state) } else { quote!() }; let with_args = if generator_state.needs_args { - quote!(let #fn_args = #deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { &*#info });) + with_fn_args(generator_state) } else { quote!() }; + let GeneratorState { + deno_core, + info, + slow_function, + .. + } = &generator_state; + Ok(quote! { pub extern "C" fn #slow_function(#info: *const #deno_core::v8::FunctionCallbackInfo) { #with_scope @@ -59,6 +56,53 @@ pub fn generate_dispatch_slow( }}) } +fn with_scope(generator_state: &mut GeneratorState) -> TokenStream { + let GeneratorState { + deno_core, + scope, + info, + .. + } = &generator_state; + + quote!(let #scope = &mut unsafe { #deno_core::v8::CallbackScope::new(&*#info) };) +} + +fn with_retval(generator_state: &mut GeneratorState) -> TokenStream { + let GeneratorState { + deno_core, + retval, + info, + .. + } = &generator_state; + + quote!(let mut #retval = #deno_core::v8::ReturnValue::from_function_callback_info(unsafe { &*#info });) +} + +fn with_fn_args(generator_state: &mut GeneratorState) -> TokenStream { + let GeneratorState { + deno_core, + fn_args, + info, + .. + } = &generator_state; + + quote!(let #fn_args = #deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { &*#info });) +} + +fn with_opctx(generator_state: &mut GeneratorState) -> TokenStream { + let GeneratorState { + deno_core, + opctx, + fn_args, + .. + } = &generator_state; + + quote!(let #opctx = unsafe { + &*(#deno_core::v8::Local::<#deno_core::v8::External>::cast(#fn_args.data()).value() + as *const #deno_core::_ops::OpCtx) + };) +} + pub fn extract_arg( generator_state: &mut GeneratorState, index: usize, @@ -176,6 +220,9 @@ pub fn return_value_infallible( } = generator_state; let res = match ret_type { + Arg::Void => { + quote! {/* void */} + } Arg::Numeric(NumericArg::u8) | Arg::Numeric(NumericArg::u16) | Arg::Numeric(NumericArg::u32) => { @@ -204,17 +251,51 @@ pub fn return_value_result( ret_type: &Arg, ) -> Result { let infallible = return_value_infallible(generator_state, ret_type)?; - let GeneratorState { result, .. } = &generator_state; + let maybe_scope = if generator_state.needs_scope { + quote!() + } else { + with_scope(generator_state) + }; + + let maybe_args = if generator_state.needs_args { + quote!() + } else { + with_fn_args(generator_state) + }; + + let maybe_opctx = if generator_state.needs_opctx { + quote!() + } else { + with_opctx(generator_state) + }; + + let GeneratorState { + deno_core, + result, + scope, + opctx, + .. + } = &generator_state; let tokens = quote!( - let result = match ret_type { + match #result { Ok(#result) => { - #infallible, + #infallible } Err(err) => { + #maybe_scope + #maybe_args + #maybe_opctx + let opstate = ::std::cell::RefCell::borrow(&*#opctx.state); + let exception = #deno_core::error::to_v8_error( + #scope, + opstate.get_error_class_fn, + &err, + ); + scope.throw_exception(exception); return; } - } + }; ); Ok(tokens) } diff --git a/ops/op2/generator_state.rs b/ops/op2/generator_state.rs index 741d4f7f33..16249c217f 100644 --- a/ops/op2/generator_state.rs +++ b/ops/op2/generator_state.rs @@ -19,6 +19,8 @@ pub struct GeneratorState { pub info: Ident, /// The `v8::FunctionCallbackArguments` used to pass args into the slow function. pub fn_args: Ident, + /// The `OpCtx` used for various information required for some ops. + pub opctx: Ident, /// The `v8::ReturnValue` used in the slow function pub retval: Ident, /// The "slow" function (ie: the one that isn't a fastcall) @@ -29,4 +31,6 @@ pub struct GeneratorState { pub needs_args: bool, pub needs_retval: bool, pub needs_scope: bool, + pub needs_opstate: bool, + pub needs_opctx: bool, } diff --git a/ops/op2/mod.rs b/ops/op2/mod.rs index 73a457f25c..558d7c7dcd 100644 --- a/ops/op2/mod.rs +++ b/ops/op2/mod.rs @@ -132,11 +132,12 @@ fn generate_op2( let fn_args = Ident::new("args", Span::call_site()); let scope = Ident::new("scope", Span::call_site()); let info = Ident::new("info", Span::call_site()); + let opctx = Ident::new("opctx", Span::call_site()); let slow_function = Ident::new("slow_function", Span::call_site()); let fast_function = Ident::new("fast_function", Span::call_site()); let deno_core = if config.core { - syn2::parse_str::("crate::deno_core") + syn2::parse_str::("crate") } else { syn2::parse_str::("deno_core") } @@ -149,6 +150,7 @@ fn generate_op2( call, scope, info, + opctx, deno_core, result, retval, @@ -157,6 +159,8 @@ fn generate_op2( fast_function, needs_retval: false, needs_scope: false, + needs_opctx: false, + needs_opstate: false, }; let name = func.sig.ident; diff --git a/ops/op2/test_cases/sync/add_options.out b/ops/op2/test_cases/sync/add_options.out index 682a773095..ca1da8fbeb 100644 --- a/ops/op2/test_cases/sync/add_options.out +++ b/ops/op2/test_cases/sync/add_options.out @@ -4,8 +4,8 @@ impl op_test_add_option { pub const fn name() -> &'static str { stringify!(op_test_add_option) } - pub const fn decl() -> crate::deno_core::_ops::OpDecl { - crate::deno_core::_ops::OpDecl { + pub const fn decl() -> crate::_ops::OpDecl { + crate::_ops::OpDecl { name: stringify!(op_test_add_option), v8_fn_ptr: Self::slow_function as _, enabled: true, @@ -16,22 +16,20 @@ impl op_test_add_option { arg_count: 2usize as u8, } } - pub extern "C" fn slow_function( - info: *const crate::deno_core::v8::FunctionCallbackInfo, - ) { - let mut rv = crate::deno_core::v8::ReturnValue::from_function_callback_info(unsafe { + pub extern "C" fn slow_function(info: *const crate::v8::FunctionCallbackInfo) { + let mut rv = crate::v8::ReturnValue::from_function_callback_info(unsafe { &*info }); - let args = crate::deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { + let args = crate::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { &*info }); let arg0 = args.get(0usize as i32); - let arg0 = crate::deno_core::_ops::to_u32(&arg0) as _; + let arg0 = crate::_ops::to_u32(&arg0) as _; let arg1 = args.get(1usize as i32); let arg1 = if arg1.is_null_or_undefined() { None } else { - let arg1 = crate::deno_core::_ops::to_u32(&arg1) as _; + let arg1 = crate::_ops::to_u32(&arg1) as _; Some(arg1) }; let result = Self::call(arg0, arg1); diff --git a/ops/op2/test_cases/sync/result_primitive.out b/ops/op2/test_cases/sync/result_primitive.out new file mode 100644 index 0000000000..a8ac50174c --- /dev/null +++ b/ops/op2/test_cases/sync/result_primitive.out @@ -0,0 +1,50 @@ +#[allow(non_camel_case_types)] +pub struct op_u32_with_result {} +impl op_u32_with_result { + pub const fn name() -> &'static str { + stringify!(op_u32_with_result) + } + pub const fn decl() -> deno_core::_ops::OpDecl { + deno_core::_ops::OpDecl { + name: stringify!(op_u32_with_result), + v8_fn_ptr: Self::slow_function as _, + enabled: true, + fast_fn: None, + is_async: false, + is_unstable: false, + is_v8: false, + arg_count: 0usize as u8, + } + } + pub extern "C" fn slow_function(info: *const deno_core::v8::FunctionCallbackInfo) { + let mut rv = deno_core::v8::ReturnValue::from_function_callback_info(unsafe { + &*info + }); + let result = Self::call(); + match result { + Ok(result) => { + rv.set_uint32(result as u32); + } + Err(err) => { + let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) }; + let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { + &*info + }); + let opctx = unsafe { + &*(deno_core::v8::Local::::cast(args.data()) + .value() as *const deno_core::_ops::OpCtx) + }; + let opstate = ::std::cell::RefCell::borrow(&*opctx.state); + let exception = deno_core::error::to_v8_error( + scope, + opstate.get_error_class_fn, + &err, + ); + scope.throw_exception(exception); + return; + } + }; + } + #[inline(always)] + pub fn call() -> Result {} +} diff --git a/ops/op2/test_cases/sync/result_primitive.rs b/ops/op2/test_cases/sync/result_primitive.rs new file mode 100644 index 0000000000..6f68fa2286 --- /dev/null +++ b/ops/op2/test_cases/sync/result_primitive.rs @@ -0,0 +1,4 @@ +// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. + +#[op2] +pub fn op_u32_with_result() -> Result {} diff --git a/ops/op2/test_cases/sync/result_void.out b/ops/op2/test_cases/sync/result_void.out new file mode 100644 index 0000000000..74c0c66a66 --- /dev/null +++ b/ops/op2/test_cases/sync/result_void.out @@ -0,0 +1,45 @@ +#[allow(non_camel_case_types)] +pub struct op_void_with_result {} +impl op_void_with_result { + pub const fn name() -> &'static str { + stringify!(op_void_with_result) + } + pub const fn decl() -> deno_core::_ops::OpDecl { + deno_core::_ops::OpDecl { + name: stringify!(op_void_with_result), + v8_fn_ptr: Self::slow_function as _, + enabled: true, + fast_fn: None, + is_async: false, + is_unstable: false, + is_v8: false, + arg_count: 0usize as u8, + } + } + pub extern "C" fn slow_function(info: *const deno_core::v8::FunctionCallbackInfo) { + let result = Self::call(); + match result { + Ok(result) => {} + Err(err) => { + let scope = &mut unsafe { deno_core::v8::CallbackScope::new(&*info) }; + let args = deno_core::v8::FunctionCallbackArguments::from_function_callback_info(unsafe { + &*info + }); + let opctx = unsafe { + &*(deno_core::v8::Local::::cast(args.data()) + .value() as *const deno_core::_ops::OpCtx) + }; + let opstate = ::std::cell::RefCell::borrow(&*opctx.state); + let exception = deno_core::error::to_v8_error( + scope, + opstate.get_error_class_fn, + &err, + ); + scope.throw_exception(exception); + return; + } + }; + } + #[inline(always)] + pub fn call() -> Result<(), AnyError> {} +} diff --git a/ops/op2/test_cases/sync/result_void.rs b/ops/op2/test_cases/sync/result_void.rs new file mode 100644 index 0000000000..41256e8c4e --- /dev/null +++ b/ops/op2/test_cases/sync/result_void.rs @@ -0,0 +1,4 @@ +// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. + +#[op2] +pub fn op_void_with_result() -> Result<(), AnyError> {}