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.

Sign up for GitHub

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

Jump to bottom

[browser][MT] JSImport dispatch to target thread via JSSynchronizationContext #96319

Merged
merged 11 commits into from
Jan 3, 2024
Merged

[browser][MT] JSImport dispatch to target thread via JSSynchronizationContext #96319

merged 11 commits into from
Jan 3, 2024

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Dec 26, 2023 •
edited
Loading

JSImport dispatch

  • JSImport will now dispatch the call to the correct thread
    • detected by affinity of JSObject passed as parameters
    • fallback to current JSWebWorker
    • fallback to UI thread
  • the dispatch would be blocking the caller for all method signatures except those returning Task/Promise
  • dispatch of calls with Task/Promise result
    • synchronously if the target thread is current thread
    • asynchronously (at the level of the interop buffer/stack frame) in all other cases
      • this is much more asynchronous than normal async call as the caller could return before the callee received the call.
  • JSImport metadata/signature buffer is now sent as parameter with the dispatched call
    • because during signature construction is not clear what is the target thread
    • JSImportGenerator doesn't generate ThreadStatic anymore for the binding variable

Tests

  • new tests in System.Runtime.InteropServices.JavaScript.Tests.WebWorkerTest, more to come later
  • System.Runtime.InteropServices.JavaScript.Tests now also run xunit on thread pool in MT

Other

  • dropped AssertIsInteropThread
  • added JSFunctionBinding.FunctionName only in Debug build
  • new tests for JSWebWorker
  • mono_set_thread_id improves TID in JS console logging from threads
  • renamed mono_wasm_main_thread_ptr
  • fixed worker.pthread_ptr

Follow up in #95370

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

ghost commented Dec 26, 2023

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

Issue Details

JSImport dispatch

  • JSImport will now dispatch the call to the correct thread
    • detected by affinity of JSObject passed as parameters
    • fallback to current JSWebWorker
    • fallback to UI thread
  • the dispatch would be blocking the caller for all method signatures except those returning Task/Promise
  • dispatch of calls with Task/Promise result
    • synchronously if the target thread is current thread
    • asynchronously (at the level of the interop buffer/stack frame) in all other cases
      • this is much more asynchronous than normal async call as the caller could return before the callee received the call.
  • JSImport metadata/signature buffer is now sent as parameter with the dispatched call
    • because during signature construction is not clear what is the target thread
    • JSImportGenerator doesn't generate ThreadStatic anymore for the binding variable

Tests

  • new tests in System.Runtime.InteropServices.JavaScript.Tests.WebWorkerTest, more to come later
  • System.Runtime.InteropServices.JavaScript.Tests now also run xunit on thread pool in MT

Other

  • dropped AssertIsInteropThread
  • added JSFunctionBinding.FunctionName only in Debug build
  • new tests for JSWebWorker
  • mono_set_thread_id improves TID in JS console logging from threads
  • renamed mono_wasm_main_thread_ptr
  • fixed worker.pthread_ptr
Author: pavelsavara
Assignees: pavelsavara
Labels:

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

@pavelsavara pavelsavara mentioned this pull request Dec 26, 2023
Merged
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara mentioned this pull request Dec 26, 2023
Closed
31 tasks
This was referenced Dec 26, 2023
Closed
Open
Open
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@pavelsavara pavelsavara marked this pull request as ready for review December 27, 2023 08:52
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

kg
kg reviewed Jan 3, 2024
View reviewed changes
...nteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSFunctionBinding.cs
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static unsafe void ResolveOrRejectPromise(JSProxyContext targetContext, Span<JSMarshalerArgument> arguments)
{
// this copy is freed in mono_wasm_invoke_import_async
Copy link
Member

Choose a reason for hiding this comment

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

this used to be always synchronous, and now becomes always-asynchronous. should we preserve the old synchronous execution when the target context is the current context?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that JS Promise handlers always go through microtask queue. So in effect this was already async before from user code perspective. We could avoid allocating C# queue item if we do the optimization you suggest.

I want to switch this to emscripten dispatch later anyway. I will add comment and keep this open question for now.

kg
kg approved these changes Jan 3, 2024
View reviewed changes
Copy link
Member

@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 this all looks good to me, and i feel like i understand most of it

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

There is known #94821 and #96191 in the CI build

@pavelsavara pavelsavara merged commit 1337d7a into dotnet:main Jan 3, 2024
134 of 139 checks passed
@pavelsavara pavelsavara deleted the browser_jsimport_just_dispatch branch January 3, 2024 16:09
This was referenced Jan 3, 2024
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilonatommy ilonatommy ilonatommy left review comments

@campersau campersau campersau left review comments

@kg kg kg approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@maraf maraf Awaiting requested review from maraf

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pavelsavara @kg @campersau @ilonatommy