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

[browser] eager allocation of Promise/Task result for async JSImport/JSExport #95411

Merged
merged 4 commits into from
Dec 2, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Nov 29, 2023

Motivation

"Because the caller already knows they need to create a proxy-promise, which would be later bound to result of the called function."

This makes it possible to send marshaling "stack frame" of async JSImport/JSExport methods as one-way message.
Because Promise/Task result is created by sender thread ahead of time and sending thread could await it even before receiving thread saw the message.

This PR doesn't contain the MT changes which will use this.

Summary

PR #93648 introduced virtual GC and JS handles.
They are good because we could allocate the handle to the future object of the other side, without crossing to that other side to create the real object. That matters in MT scenarios, in which we also need to cross to the correct thread.

This PR optimizes it further by allocating the real PromiseHolder or TaskHolder and normal non-virtual handle for it.
We do it for the return value of async JSImport or JSExport.

The Task/Promise passed as parameter keep using the virtual handles.
"Because we only need to pass virtual identity of the proxy-promise which will be created on the other side".

We still support case when target method returns synchronously resolved Task/Promise or null. Only when caller invokes the JSImport/JSExport synchronously. Which is currently always.

null value and synchronous resolve/reject would not be possible return value of real async cross-thread MT dispatch. There are currently WS client scenarios which are using synchronous null as signal. That would need to be fixed on another PR.

Changes

  • pre-allocate PromiseHolder for async JSImport in InvokeJSImportImpl
  • rename CreateJSOwnedHolder to GetPromiseHolder
  • renames back from gcvHandle to gcHandle where both could be used
  • rename InvokeJSImpl to InvokeJSFunction
  • rename mono_wasm_invoke_bound_function to mono_wasm_invoke_js_function
  • use OwnerTID in JSObject instead of ManagedThreadId
  • make JSObject.Dispose asynchronously Posted to thread of affinity.
  • added async methods to the advanced sample

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Nov 29, 2023
@pavelsavara pavelsavara added this to the 9.0.0 milestone Nov 29, 2023
@pavelsavara pavelsavara self-assigned this Nov 29, 2023
@ghost
Copy link

ghost commented Nov 29, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Motivation

This makes it possible to send marshaling "stack frame" of async JSImport/JSExport methods as message.
Because Promise/Task result is created by sender thread ahead of time and sending thread could await it even before receiving thread saw the message.

This PR doesn't contain the MT changes which will use this.

Summary

PR #93648 introduced virtual GC and JS handles.
They are good because we could allocate the handle to the future object of the other side, without crossing to that other side to create the real object. That matters in MT scenarios, in which we also need to cross to the correct thread.

This PR optimizes it further by allocating the real PromiseHolder or TaskHolder and normal non-virtual handle for it.
We do it for the return value of async JSImport or JSExport.
The Task/Promise passed as parameter keep using the virtual handles.

This supports case target method synchronously returns null Task/Promise, only when caller invokes the JSImport/JSExport synchronously. Which is currently always.

Changes

  • pre-allocate PromiseHolder for async JSImport in InvokeJSImportImpl

  • rename CreateJSOwnedHolder to GetPromiseHolder

  • renames back from gcvHandle to gcHandle where both could be used

  • rename InvokeJSImpl to InvokeJSFunction

  • rename mono_wasm_invoke_bound_function to mono_wasm_invoke_js_function

  • use OwnerTID in JSObject instead of ManagedThreadId

  • make JSObject.Dispose asynchronously Posted to thread of affinity.

  • added async methods to the advanced sample

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, os-browser

Milestone: 9.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@kg kg left a comment

Choose a reason for hiding this comment

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

Other than my comments I don't see anything blocking this, but I don't feel confident at all in my understanding of jsv/gcv vs js/gc handles or the correctness of the code using them

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

There is unrelated System.Collections.Concurrent.Tests in WASM MT.

@pavelsavara pavelsavara merged commit 5004872 into dotnet:main Dec 2, 2023
133 of 135 checks passed
@pavelsavara pavelsavara deleted the browser_eager_promise branch December 2, 2023 14:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants