Skip to content

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

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
Merged
pavelsavara merged 11 commits into from
Jan 3, 2024
Merged

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

pavelsavara 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

pavelsavara commented Dec 26, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 26, 2023

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
Closed
Closed
@pavelsavara
Copy link
Member Author

pavelsavara commented Dec 27, 2023

/azp run runtime-wasm

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

azure-pipelines bot commented Dec 27, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Dec 30, 2023

/azp run runtime-wasm

kg
kg reviewed Jan 3, 2024
View reviewed changes
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

@kg kg Jan 3, 2024

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

@pavelsavara pavelsavara Jan 3, 2024

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 reviewed Jan 3, 2024
View reviewed changes
kg
kg reviewed Jan 3, 2024
View reviewed changes
kg
kg reviewed Jan 3, 2024
View reviewed changes
kg
kg reviewed Jan 3, 2024
View reviewed changes
kg
kg reviewed Jan 3, 2024
View reviewed changes
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

pavelsavara commented Jan 3, 2024

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 3, 2024

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 3, 2024

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

@pavelsavara pavelsavara merged commit 1337d7a into dotnet:main Jan 3, 2024
@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

@kg kg kg approved these changes

@ilonatommy ilonatommy ilonatommy left review comments

@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

+1 more reviewer

@campersau campersau campersau left review comments

Reviewers whose approvals may not affect merge requirements

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