perf(ops): Reenable fast unit result optimization (denoland#16827)

The optimization was missed in the optimizer rewrite denoland#16514
DavidDeSimone · Nov 27, 2022 · 9ffc6ac · 9ffc6ac
1 parent 0012484
commit 9ffc6ac
Show file tree

Hide file tree

Showing 35 changed files with 844 additions and 32 deletions.
diff --git a/ext/net/ops.rs b/ext/net/ops.rs
@@ -67,8 +67,8 @@ pub fn init<P: NetPermissions + 'static>() -> Vec<OpDecl> {
     #[cfg(unix)]
     crate::ops_unix::op_net_send_unixpacket::decl::<P>(),
     op_dns_resolve::decl::<P>(),
-    op_set_nodelay::decl::<P>(),
-    op_set_keepalive::decl::<P>(),
+    op_set_nodelay::decl(),
+    op_set_keepalive::decl(),
   ]
 }
 
@@ -509,7 +509,7 @@ where
 }
 
 #[op]
-pub fn op_set_nodelay<NP>(
+pub fn op_set_nodelay(
   state: &mut OpState,
   rid: ResourceId,
   nodelay: bool,
@@ -521,7 +521,7 @@ pub fn op_set_nodelay<NP>(
 }
 
 #[op]
-pub fn op_set_keepalive<NP>(
+pub fn op_set_keepalive(
   state: &mut OpState,
   rid: ResourceId,
   keepalive: bool,
@@ -836,7 +836,7 @@ mod tests {
   #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
   async fn tcp_set_no_delay() {
     let set_nodelay = Box::new(|state: &mut OpState, rid| {
-      op_set_nodelay::call::<TestPermission>(state, rid, true).unwrap();
+      op_set_nodelay::call(state, rid, true).unwrap();
     });
     let test_fn = Box::new(|socket: SockRef| {
       assert!(socket.nodelay().unwrap());
@@ -848,7 +848,7 @@ mod tests {
   #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
   async fn tcp_set_keepalive() {
     let set_keepalive = Box::new(|state: &mut OpState, rid| {
-      op_set_keepalive::call::<TestPermission>(state, rid, true).unwrap();
+      op_set_keepalive::call(state, rid, true).unwrap();
     });
     let test_fn = Box::new(|socket: SockRef| {
       assert!(!socket.nodelay().unwrap());

diff --git a/ops/fast_call.rs b/ops/fast_call.rs
@@ -141,10 +141,19 @@ pub(crate) fn generate(
   if optimizer.has_fast_callback_option
     || optimizer.needs_opstate()
     || optimizer.is_async
+    || optimizer.needs_fast_callback_option
   {
-    fast_fn_inputs.push(parse_quote! {
+    let decl = parse_quote! {
       fast_api_callback_options: *mut #core::v8::fast_api::FastApiCallbackOptions
-    });
+    };
+
+    if optimizer.has_fast_callback_option {
+      // Replace last parameter.
+      assert!(fast_fn_inputs.pop().is_some());
+      fast_fn_inputs.push(decl);
+    } else {
+      fast_fn_inputs.push(decl);
+    }
 
     input_variants.push(q!({ CallbackOptions }));
   }
@@ -162,14 +171,10 @@ pub(crate) fn generate(
 
   let mut output_transforms = q!({});
 
-  if optimizer.needs_opstate() || optimizer.is_async {
-    // Grab the op_state identifier, the first one. ¯\_(ツ)_/¯
-    let op_state = match idents.first() {
-      Some(ident) if optimizer.has_opstate_in_parameters() => ident.clone(),
-      // fn op_foo() -> Result<...>
-      _ => Ident::new("op_state", Span::call_site()),
-    };
-
+  if optimizer.needs_opstate()
+    || optimizer.is_async
+    || optimizer.has_fast_callback_option
+  {
     // Dark arts 🪄 ✨
     //
     // - V8 calling convention guarantees that the callback options pointer is non-null.
@@ -179,13 +184,27 @@ pub(crate) fn generate(
     let prelude = q!({
       let __opts: &mut v8::fast_api::FastApiCallbackOptions =
         unsafe { &mut *fast_api_callback_options };
+    });
+
+    pre_transforms.push_tokens(&prelude);
+  }
+
+  if optimizer.needs_opstate() || optimizer.is_async {
+    // Grab the op_state identifier, the first one. ¯\_(ツ)_/¯
+    let op_state = match idents.first() {
+      Some(ident) if optimizer.has_opstate_in_parameters() => ident.clone(),
+      // fn op_foo() -> Result<...>
+      _ => Ident::new("op_state", Span::call_site()),
+    };
+
+    let ctx = q!({
       let __ctx = unsafe {
         &*(v8::Local::<v8::External>::cast(unsafe { __opts.data.data }).value()
           as *const _ops::OpCtx)
       };
     });
 
-    pre_transforms.push_tokens(&prelude);
+    pre_transforms.push_tokens(&ctx);
     pre_transforms.push_tokens(&match optimizer.is_async {
       false => q!(
         Vars {

diff --git a/ops/lib.rs b/ops/lib.rs
@@ -76,10 +76,11 @@ impl Op {
   fn gen(mut self) -> TokenStream2 {
     let mut optimizer = Optimizer::new();
     match optimizer.analyze(&mut self) {
-      Ok(_) | Err(BailoutReason::MustBeSingleSegment) => {}
-      Err(BailoutReason::FastUnsupportedParamType) => {
+      Err(BailoutReason::MustBeSingleSegment)
+      | Err(BailoutReason::FastUnsupportedParamType) => {
         optimizer.fast_compatible = false;
       }
+      _ => {}
     };
 
     let Self {

diff --git a/ops/optimizer.rs b/ops/optimizer.rs
@@ -9,7 +9,7 @@ use syn::{
   parse_quote, punctuated::Punctuated, token::Colon2,
   AngleBracketedGenericArguments, FnArg, GenericArgument, PatType, Path,
   PathArguments, PathSegment, ReturnType, Signature, Type, TypePath, TypePtr,
-  TypeReference, TypeSlice,
+  TypeReference, TypeSlice, TypeTuple,
 };
 
 #[derive(Debug)]
@@ -196,7 +196,10 @@ pub(crate) struct Optimizer {
 
   pub(crate) has_rc_opstate: bool,
 
+  // Do we need an explict FastApiCallbackOptions argument?
   pub(crate) has_fast_callback_option: bool,
+  // Do we depend on FastApiCallbackOptions?
+  pub(crate) needs_fast_callback_option: bool,
 
   pub(crate) fast_result: Option<FastValue>,
   pub(crate) fast_parameters: Vec<FastValue>,
@@ -218,6 +221,11 @@ impl Debug for Optimizer {
       "has_fast_callback_option: {}",
       self.has_fast_callback_option
     )?;
+    writeln!(
+      f,
+      "needs_fast_callback_option: {}",
+      self.needs_fast_callback_option
+    )?;
     writeln!(f, "fast_result: {:?}", self.fast_result)?;
     writeln!(f, "fast_parameters: {:?}", self.fast_parameters)?;
     writeln!(f, "transforms: {:?}", self.transforms)?;
@@ -298,6 +306,9 @@ impl Optimizer {
 
   fn analyze_return_type(&mut self, ty: &Type) -> Result<(), BailoutReason> {
     match ty {
+      Type::Tuple(TypeTuple { elems, .. }) if elems.is_empty() => {
+        self.fast_result = Some(FastValue::Void);
+      }
       Type::Path(TypePath {
         path: Path { segments, .. },
         ..
@@ -333,6 +344,14 @@ impl Optimizer {
                   self.fast_compatible = false;
                   return Err(BailoutReason::FastUnsupportedParamType);
                 }
+                Some(GenericArgument::Type(Type::Tuple(TypeTuple {
+                  elems,
+                  ..
+                })))
+                  if elems.is_empty() =>
+                {
+                  self.fast_result = Some(FastValue::Void);
+                }
                 _ => return Err(BailoutReason::FastUnsupportedParamType),
               }
             }
@@ -407,15 +426,19 @@ impl Optimizer {
                   {
                     let segment = single_segment(segments)?;
                     match segment {
-                      // Is `T` a FastApiCallbackOption?
+                      // Is `T` a FastApiCallbackOptions?
                       PathSegment { ident, .. }
-                        if ident == "FastApiCallbackOption" =>
+                        if ident == "FastApiCallbackOptions" =>
                       {
                         self.has_fast_callback_option = true;
                       }
-                      _ => {}
+                      _ => return Err(BailoutReason::FastUnsupportedParamType),
                     }
+                  } else {
+                    return Err(BailoutReason::FastUnsupportedParamType);
                   }
+                } else {
+                  return Err(BailoutReason::FastUnsupportedParamType);
                 }
               }
             }
@@ -517,7 +540,7 @@ impl Optimizer {
               match segment {
                 // Is `T` a u8?
                 PathSegment { ident, .. } if ident == "u8" => {
-                  self.has_fast_callback_option = true;
+                  self.needs_fast_callback_option = true;
                   self.fast_parameters.push(FastValue::Uint8Array);
                   assert!(self
                     .transforms
@@ -526,7 +549,7 @@ impl Optimizer {
                 }
                 // Is `T` a u32?
                 PathSegment { ident, .. } if ident == "u32" => {
-                  self.has_fast_callback_option = true;
+                  self.needs_fast_callback_option = true;
                   self.fast_parameters.push(FastValue::Uint32Array);
                   assert!(self
                     .transforms
@@ -554,7 +577,7 @@ impl Optimizer {
             match segment {
               // Is `T` a u8?
               PathSegment { ident, .. } if ident == "u8" => {
-                self.has_fast_callback_option = true;
+                self.needs_fast_callback_option = true;
                 self.fast_parameters.push(FastValue::Uint8Array);
                 assert!(self
                   .transforms

diff --git a/ops/optimizer_tests/async_nop.expected b/ops/optimizer_tests/async_nop.expected
@@ -3,6 +3,7 @@ returns_result: false
 has_ref_opstate: false
 has_rc_opstate: false
 has_fast_callback_option: false
+needs_fast_callback_option: false
 fast_result: Some(Void)
 fast_parameters: [V8Value, I32]
 transforms: {}

diff --git a/ops/optimizer_tests/async_result.expected b/ops/optimizer_tests/async_result.expected
@@ -2,7 +2,8 @@
 returns_result: true
 has_ref_opstate: false
 has_rc_opstate: true
-has_fast_callback_option: true
+has_fast_callback_option: false
+needs_fast_callback_option: true
 fast_result: None
 fast_parameters: [V8Value, I32, U32, Uint8Array]
 transforms: {2: Transform { kind: SliceU8(true), index: 2 }}

diff --git a/ops/optimizer_tests/callback_options.expected b/ops/optimizer_tests/callback_options.expected
@@ -2,7 +2,8 @@
 returns_result: false
 has_ref_opstate: false
 has_rc_opstate: false
-has_fast_callback_option: false
+has_fast_callback_option: true
+needs_fast_callback_option: false
 fast_result: Some(Void)
 fast_parameters: [V8Value]
 transforms: {}

diff --git a/ops/optimizer_tests/callback_options.out b/ops/optimizer_tests/callback_options.out
@@ -61,18 +61,21 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_fallback_fast {
     fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
         use deno_core::v8::fast_api::Type::*;
         use deno_core::v8::fast_api::CType;
-        &[V8Value]
+        &[V8Value, CallbackOptions]
     }
     fn return_type(&self) -> deno_core::v8::fast_api::CType {
         deno_core::v8::fast_api::CType::Void
     }
 }
 fn op_fallback_fast_fn<'scope>(
     _: deno_core::v8::Local<deno_core::v8::Object>,
-    options: Option<&mut FastApiCallbackOptions>,
+    fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
 ) -> () {
     use deno_core::v8;
     use deno_core::_ops;
+    let __opts: &mut v8::fast_api::FastApiCallbackOptions = unsafe {
+        &mut *fast_api_callback_options
+    };
     let result = op_fallback::call(options);
     result
 }
diff --git a/ops/optimizer_tests/op_blob_revoke_object_url.expected b/ops/optimizer_tests/op_blob_revoke_object_url.expected
@@ -0,0 +1 @@
+MustBeSingleSegment
diff --git a/ops/optimizer_tests/op_blob_revoke_object_url.out b/ops/optimizer_tests/op_blob_revoke_object_url.out
@@ -0,0 +1,71 @@
+#[allow(non_camel_case_types)]
+///Auto-generated by `deno_ops`, i.e: `#[op]`
+///
+///Use `op_blob_revoke_object_url::decl()` to get an op-declaration
+///you can include in a `deno_core::Extension`.
+pub struct op_blob_revoke_object_url;
+#[doc(hidden)]
+impl op_blob_revoke_object_url {
+    pub fn name() -> &'static str {
+        stringify!(op_blob_revoke_object_url)
+    }
+    pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback {
+        use deno_core::v8::MapFnTo;
+        Self::v8_func.map_fn_to()
+    }
+    pub fn decl<'scope>() -> deno_core::OpDecl {
+        deno_core::OpDecl {
+            name: Self::name(),
+            v8_fn_ptr: Self::v8_fn_ptr(),
+            enabled: true,
+            fast_fn: None,
+            is_async: false,
+            is_unstable: false,
+            is_v8: false,
+            argc: 1usize,
+        }
+    }
+    #[inline]
+    #[allow(clippy::too_many_arguments)]
+    pub fn call(state: &mut deno_core::OpState, url: String) -> Result<(), AnyError> {
+        let url = Url::parse(&url)?;
+        let blob_store = state.borrow::<BlobStore>();
+        blob_store.remove_object_url(&url);
+        Ok(())
+    }
+    pub fn v8_func<'scope>(
+        scope: &mut deno_core::v8::HandleScope<'scope>,
+        args: deno_core::v8::FunctionCallbackArguments,
+        mut rv: deno_core::v8::ReturnValue,
+    ) {
+        let ctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        let arg_0 = match deno_core::v8::Local::<
+            deno_core::v8::String,
+        >::try_from(args.get(0usize as i32)) {
+            Ok(v8_string) => deno_core::serde_v8::to_utf8(v8_string, scope),
+            Err(_) => {
+                return deno_core::_ops::throw_type_error(
+                    scope,
+                    format!("Expected string at position {}", 0usize),
+                );
+            }
+        };
+        let result = Self::call(&mut std::cell::RefCell::borrow_mut(&ctx.state), arg_0);
+        let op_state = ::std::cell::RefCell::borrow(&*ctx.state);
+        op_state.tracker.track_sync(ctx.id);
+        match result {
+            Ok(result) => {}
+            Err(err) => {
+                let exception = deno_core::error::to_v8_error(
+                    scope,
+                    op_state.get_error_class_fn,
+                    &err,
+                );
+                scope.throw_exception(exception);
+            }
+        };
+    }
+}
diff --git a/ops/optimizer_tests/op_blob_revoke_object_url.rs b/ops/optimizer_tests/op_blob_revoke_object_url.rs
@@ -0,0 +1,9 @@
+pub fn op_blob_revoke_object_url(
+  state: &mut deno_core::OpState,
+  url: String,
+) -> Result<(), AnyError> {
+  let url = Url::parse(&url)?;
+  let blob_store = state.borrow::<BlobStore>();
+  blob_store.remove_object_url(&url);
+  Ok(())
+}
diff --git a/ops/optimizer_tests/op_state.expected b/ops/optimizer_tests/op_state.expected
@@ -3,6 +3,7 @@ returns_result: false
 has_ref_opstate: true
 has_rc_opstate: false
 has_fast_callback_option: false
+needs_fast_callback_option: false
 fast_result: Some(Void)
 fast_parameters: [V8Value, I32]
 transforms: {}

diff --git a/ops/optimizer_tests/op_state_basic1.expected b/ops/optimizer_tests/op_state_basic1.expected
@@ -3,6 +3,7 @@ returns_result: false
 has_ref_opstate: true
 has_rc_opstate: false
 has_fast_callback_option: false
+needs_fast_callback_option: false
 fast_result: Some(U32)
 fast_parameters: [V8Value, U32, U32]
 transforms: {}

diff --git a/ops/optimizer_tests/op_state_generics.expected b/ops/optimizer_tests/op_state_generics.expected
@@ -3,6 +3,7 @@ returns_result: false
 has_ref_opstate: true
 has_rc_opstate: false
 has_fast_callback_option: false
+needs_fast_callback_option: false
 fast_result: Some(Void)
 fast_parameters: [V8Value]
 transforms: {}