Skip to content

[browser][MT] dispatch across threads via emscripten - #97669

Merged
pavelsavara merged 8 commits into
dotnet:mainfrom
pavelsavara:browser_mt_emscripten_dispatch
Feb 5, 2024
Merged

[browser][MT] dispatch across threads via emscripten#97669
pavelsavara merged 8 commits into
dotnet:mainfrom
pavelsavara:browser_mt_emscripten_dispatch

Conversation

@pavelsavara

@pavelsavarapavelsavara commented Jan 29, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Dispatch

use emscripten job queue in the target thread instead of JSSynchronizationContext to call cross-thread JS functions.

This has following benefits

  • the target thread doesn't have to be managed thread.
    • this contributes to the goal of avoiding managed code on UI thread to avoid clashing with GC stop-the-world
  • smaller allocation of the job object
  • probably faster than interp of the JSSynchronizationContext
  • doesn't need to wait for schedule_background_exec -> setTimeout tick

Cross-thread asynchronous

All of them are fire-and-forget messages

  • ReleaseCSOwnedObjectPost
  • InvokeJSImportAsyncPost
  • ResolveOrRejectPromisePost & CancelPromisePost
    • this is complex case because there could be (rare) racing threads.
    • The one with JS objects is resolving the Task/Promise and another thread is triggering cancellation.
    • We need to keep the GCHandle of the Task/Promise alive so, that the async messages don't have use-after-free problem.
    • After this PR we don't have the use-after-free problem
    • But we leak the GCHandle instead, only when there is a race condition.
    • The problem will become even more complex, once we want to also resolve/reject the promise->task by async message.

Cross-thread synchronous

  • InvokeJSFunctionSend & InvokeJSImportSyncSend
    • both of them are blocking the caller in native code via GC friendly mono_coop_sem_wait
    • I speculate that target thread is unable to .Wait for too long
  • JSSynchronizationContext.Send now throws PNSE on JS threads via ThrowOnBlockingWaitOnJSInteropThread
  • ConsolePal.Clear - ICAL instead of JSImport - more in line the rest of with posix console pal.

Other

  • deleted legacy interop ICAL dead code

@pavelsavarapavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Jan 29, 2024
@pavelsavarapavelsavara added this to the 9.0.0 milestone Jan 29, 2024
@pavelsavarapavelsavara self-assigned this Jan 29, 2024
@ghost

ghost commented Jan 29, 2024

Copy link
Copy Markdown

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

Issue Details

null

Author:pavelsavara
Assignees:pavelsavara
Labels:

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

Milestone:9.0.0

@build-analysisbuild-analysisBot mentioned this pull request Jan 29, 2024
Closed
@pavelsavara
pavelsavaraforce-pushed the browser_mt_emscripten_dispatch branch 2 times, most recently from ef0ff17 to 0d4b99fCompareJanuary 30, 2024 18:37
This was referenced Jan 30, 2024
Closed
Closed
@pavelsavara
pavelsavaraforce-pushed the browser_mt_emscripten_dispatch branch from 0d4b99f to cd460dbCompareJanuary 31, 2024 08:07
This was referenced Jan 31, 2024
Open
Open
Closed
Open
Closed
@pavelsavara

This comment was marked as resolved.

Sign in to view
@pavelsavara

pavelsavara commented Jan 31, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

azure-pipelinesBot commented Jan 31, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@build-analysisbuild-analysisBot mentioned this pull request Feb 1, 2024
Closed
@pavelsavara
pavelsavaraforce-pushed the browser_mt_emscripten_dispatch branch from 98d8cf5 to e4ec20aCompareFebruary 1, 2024 14:26
@build-analysisbuild-analysisBot mentioned this pull request Feb 1, 2024
Closed
@pavelsavara
pavelsavaraforce-pushed the browser_mt_emscripten_dispatch branch from e4ec20a to 7b1df18CompareFebruary 2, 2024 18:35
@pavelsavara

pavelsavara commented Feb 2, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

azure-pipelinesBot commented Feb 2, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
pavelsavara marked this pull request as ready for review February 2, 2024 19:26
@pavelsavara
pavelsavara requested review from kg and marafFebruary 2, 2024 19:26
@pavelsavarapavelsavara mentioned this pull request Feb 2, 2024
Closed
31 tasks
@build-analysisbuild-analysisBot mentioned this pull request Feb 2, 2024
Open
maraf
maraf reviewed Feb 5, 2024
View reviewed changes
Comment thread...rvices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs
Comment thread...e.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSProxyContext.cs
Comment thread...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs Outdated
Comment thread...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs Outdated
Comment threadsrc/mono/browser/runtime/corebindings.c Outdated
pavelsavaraand others added 5 commits February 5, 2024 10:30
@pavelsavara@maraf
Update src/libraries/System.Runtime.InteropServices.JavaScript/src/Sy…
179caf5
…stem/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs
Co-authored-by: Marek Fišera <mara@neptuo.com>
@pavelsavara@maraf
Update src/libraries/System.Runtime.InteropServices.JavaScript/src/Sy…
d826d1f
…stem/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs
Co-authored-by: Marek Fišera <mara@neptuo.com>
@pavelsavara@maraf
Update src/mono/browser/runtime/corebindings.c
aefd338
Co-authored-by: Marek Fišera <mara@neptuo.com>
@pavelsavara
Merge branch 'browser_mt_emscripten_dispatch' ofhttps://github.com/p…
06361f7
…avelsavara/runtime into browser_mt_emscripten_dispatch
@pavelsavara
feedback
2789c39
@pavelsavara

pavelsavara commented Feb 5, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

azure-pipelinesBot commented Feb 5, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

maraf
maraf approved these changes Feb 5, 2024
View reviewed changes

@marafmaraf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

@pavelsavara

pavelsavara commented Feb 5, 2024

Copy link
Copy Markdown
MemberAuthor

I filled #97968

This was referenced Feb 5, 2024
Closed
Closed
@pavelsavara
pavelsavara merged commit 68a18eb into dotnet:mainFeb 5, 2024
@pavelsavara
pavelsavara deleted the browser_mt_emscripten_dispatch branch February 5, 2024 12:08
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 7, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marafmarafmaraf approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@vargazvargazAwaiting requested review from vargaz
@kgkgAwaiting requested review from kg

Assignees

@pavelsavarapavelsavara

Labels

arch-wasmWebAssembly architecturearea-System.Runtime.InteropServices.JavaScriptos-browserBrowser variant of arch-wasm

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@pavelsavara@maraf