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

Introduce a default synchronization context for wasm #72652

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

kg
Copy link
Member

@kg kg commented Jul 22, 2022

This PR introduces a JSSynchronizationContext that is installed automatically on the main thread when we initialize the bindings. It implements both Send and Post on top of emscripten's main thread call proxying API when you use it from workers, and on the main thread it just invokes the callback immediately.

I tried to keep the implementation as simple as possible without making it slow enough to produce a meaningful performance regression, since things like await will use this by default once it's merged. The queue of jobs is maintained in managed code and pumped by a dispatcher, and if we notice that the dispatcher is not currently waiting to run when we add something to the queue, we kick off a request to run the dispatcher. This means that worst-case we will run the dispatcher once per work item, but if a bunch of work items end up in the queue, they can get serviced efficiently by a single invocation of the dispatcher. And for calls from the main thread the performance should be equivalent to what it was before due to the fast-path.

@kg kg requested a review from lambdageek July 22, 2022 00:03
@ghost ghost assigned kg Jul 22, 2022
@kg kg added the arch-wasm WebAssembly architecture label Jul 22, 2022
@ghost
Copy link

ghost commented Jul 22, 2022

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

Issue Details

This PR introduces a JSSynchronizationContext that is installed automatically on the main thread when we initialize the bindings. It implements both Send and Post on top of emscripten's main thread call proxying API when you use it from workers, and on the main thread it just invokes the callback immediately.

I tried to keep the implementation as simple as possible without making it slow enough to produce a meaningful performance regression, since things like await will use this by default once it's merged. The queue of jobs is maintained in managed code and pumped by a dispatcher, and if we notice that the dispatcher is not currently waiting to run when we add something to the queue, we kick off a request to run the dispatcher. This means that worst-case we will run the dispatcher once per work item, but if a bunch of work items end up in the queue, they can get serviced efficiently by a single invocation of the dispatcher. And for calls from the main thread the performance should be equivalent to what it was before due to the fast-path.

Author: kg
Assignees: kg
Labels:

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

Milestone: -

@kg kg requested a review from lambdageek July 30, 2022 06:14
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

I took a pass through the concurrent logic and it seems ok (and I will take a more detailed look if we decide to go with it), but I'd like to understand first if it's possible to use some higher-level abstraction instead. We're setting up a channel that workers write into and that the main thread reads from. Can we use an actual channel?

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

The design makes sense to me

Found a few minor things in the implementation, but it's cleanup, not deep issues.

@kg kg force-pushed the wasm-sync-context branch 2 times, most recently from e49b79e to 1fa99ef Compare August 6, 2022 06:42

public override void Post (SendOrPostCallback d, object? state) {
var workItem = new WorkItem(d, state, null);
while (!Queue.Writer.TryWrite(workItem))
Copy link
Member

Choose a reason for hiding this comment

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

When would this ever return false? The channel is unbounded.

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation was unclear on when this could fail, and I didn't have a chance to read it yet since there are multiple queue implementations. Wasn't sure if it was a wait-free thing where TryWrite failing means 'try again, someone had the lock'

Copy link
Member

Choose a reason for hiding this comment

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

cc @IEvangelist a chance to improve the docs! Though this might be an API doc.

@kg kg marked this pull request as ready for review August 9, 2022 04:45
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM except the managed-exports.ts bits which I don't understand

src/mono/wasm/runtime/managed-exports.ts Outdated Show resolved Hide resolved
@lambdageek
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Member Author

kg commented Aug 12, 2022

Lots of failures on the wasm lanes but none of them look related to this PR. Not sure what to do about that. I see some temp file exists errors that seem to be some sort of problem with the build machine/build environment (I didn't touch native IO at all) and then a bunch of problems in websockets that appear related to SharedArrayBuffer, which I also didn't change.

@radical
Copy link
Member

radical commented Aug 12, 2022

The LibraryTests one is #73721 .
The AOT one is "like" #72752 - in the sense that it has the same unhelpful, non-specific error message!

kg added 2 commits August 12, 2022 01:41
…ion calls back to the browser thread and queues them as background jobs.

Exercise sync context in threads sample to display the complete progress indicator
Undo changes to normal browser sample
Update a comment and address PR feedback
Remove prints and update comments
Cleanup debugging changes
Migrate install method to new marshaling API and clean up merge damage
@kg kg merged commit 55f5c7c into dotnet:main Aug 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants