- 
                Notifications
    
You must be signed in to change notification settings  - Fork 132
 
          perf: resolve Promises natively for async ops
          #1163
        
          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
base: main
Are you sure you want to change the base?
Conversation
| 
           Help wanted async stacktrace for error is gone rn  
I need do the same but in native: somewhat solution: (obviously bad, but at least it kinda works) Don't have a bindings for this, also this is private i guess (https://github.com/v8/v8/blob/e3529d092163dcfbbb454257dc4103bdebfeda48/src/execution/messages.h#L150 This one should be public?, but still no binding for it i guess: https://github.com/v8/v8/blob/e3529d092163dcfbbb454257dc4103bdebfeda48/include/v8-exception.h#L69 I didn't found a considerable solutions, maybe I need to add bindings to rusty_v8 first?  | 
    
| op_unref_op(promiseId); | ||
| } | ||
| 
               | 
          ||
| function refOpPromise(promise) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self, verify purpose of refOp and refOpPromise and if they can be de-duped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since promiseId is now private so it's not able to be retrieved in JS side directly so I add ops for ref/unref promises directly
| self.promises.borrow().get(promise_id as usize).is_some() | ||
| } | ||
| 
               | 
          ||
| fn _get_promise<'s>( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self, remove if not needed before landing
| queue, | ||
| completed_waker, | ||
| arena: Default::default(), | ||
| promises: Default::default(), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be pre-allocated
| 
           @CyanChanges can you please provide result when running this code in your PR and on  From our benchmarks this PR appears to be significantly slower than   | 
    
          
 
 I just switched to NixOS recently, so I needs to configure the dev environment and rebuild it. It may take a while tho  | 
    
          
  
       | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using internalized strings and re-using the v8::Private. It cuts down some overhead but still not as fast as main:
diff --git a/core/runtime/op_driver/mod.rs b/core/runtime/op_driver/mod.rs
index 4a9eaa8..fe66a8f 100644
--- a/core/runtime/op_driver/mod.rs
+++ b/core/runtime/op_driver/mod.rs
@@ -21,7 +21,6 @@ pub use self::op_results::OpResult;
 use self::op_results::PendingOpInfo;
 pub use self::op_results::V8OpMappingContext;
 pub use self::op_results::V8RetValMapper;
-use crate::runtime::v8_static_strings::INTERNAL_PROMISE_ID;
 #[derive(Default)]
 /// Returns a set of stats on inflight ops.
@@ -37,6 +36,10 @@ pub enum OpScheduling {
   Deferred,
 }
+thread_local! {
+static PRIVATE_PROMISE_ID: std::cell::OnceCell<v8::Global<v8::Private>> = std::cell::OnceCell::new();
+}
+
 /// `OpDriver` encapsulates the interface for handling operations within Deno's runtime.
 ///
 /// This trait defines methods for submitting ops and polling readiness inside of the
@@ -55,8 +58,21 @@ pub(crate) trait OpDriver<C: OpMappingContext = V8OpMappingContext>:
     &self,
     scope: &mut v8::HandleScope<'s>,
   ) -> v8::Local<'s, v8::Private> {
-    let name = INTERNAL_PROMISE_ID.v8_string(scope).unwrap();
-    v8::Private::for_api(scope, Some(name))
+    PRIVATE_PROMISE_ID.with(move |cell| {
+      let global = cell.get_or_init(|| {
+        let internalized = v8::String::new_from_one_byte(
+          scope,
+          b"a",
+          v8::NewStringType::Internalized,
+        )
+        .unwrap();
+
+        let private = v8::Private::for_api(scope, Some(internalized));
+        v8::Global::new(scope, private)
+      });
+
+      v8::Local::new(scope, global)
+    })
   }
   fn _get_promise<'s>(
diff --git a/core/runtime/v8_static_strings.rs b/core/runtime/v8_static_strings.rs
index 1bee095..c5c0528 100644
--- a/core/runtime/v8_static_strings.rs
+++ b/core/runtime/v8_static_strings.rs
@@ -33,7 +33,6 @@ v8_static_strings!(
   NAME = "name",
   OPS = "ops",
   RESOLVE = "resolve",
-  INTERNAL_PROMISE_ID = "Promise#Deno.core.internalPromiseId",
   STACK = "stack",
   URL = "url",
   WASM_INSTANCE = "WasmInstance",$ ./dcore_main test.mjs                                                  [INS]
🛑 deno_core binary is meant for development and testing purposes.
Run test.mjs
op_void_async took 45ms
op_void_async_deferred took 1270ms
$ target/release/dcore test.mjs                                           [INS]
🛑 deno_core binary is meant for development and testing purposes.
Run test.mjs
op_void_async took 174ms
op_void_async_deferred took 1394ms





Supersede #1158
Waiting for
Missing binding forException::CaptureStackTracerusty_v8#1818Summary
22% more performant async ops,
(by resolve
Promises natively)and less memory usage (no need of preparing an array of results)