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

Wasm async entrypoint #44045

Merged
merged 9 commits into from
Nov 4, 2020
Merged

Wasm async entrypoint #44045

merged 9 commits into from
Nov 4, 2020

Conversation

lewing
Copy link
Member

@lewing lewing commented Oct 30, 2020

This checks to see if the assemblies entry point is really a wrapper around an async method and if so tries to invoke the async entry point so that we can yield back to the js loop properly.

@lewing lewing added arch-wasm WebAssembly architecture area-VM-meta-mono labels Oct 30, 2020
@ghost
Copy link

ghost commented Oct 30, 2020

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

@lewing lewing mentioned this pull request Oct 30, 2020
Merged
@lewing
Copy link
Member Author

lewing commented Oct 30, 2020 •
edited
Loading

Part of the fix for #43958

This was referenced Oct 30, 2020
Merged
Closed
@lewing lewing requested a review from kg October 30, 2020 04:35
kg
kg previously requested changes Oct 30, 2020
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.

Looks fine to me other than my one bit of feedback + we don't want to merge the changes to the console sample, do we?

@lewing
Copy link
Member Author

lewing commented Oct 30, 2020

Looks fine to me other than my one bit of feedback + we don't want to merge the changes to the console sample, do we?

It isn't really a sample, it is a runtime test see #44016

CoffeeFlux
CoffeeFlux reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Couple minor stylistic nits

@kg
Copy link
Member

kg commented Oct 30, 2020 •
edited
Loading

It isn't really a sample, it is a runtime test see #44016

Makes sense, the 1 second sleep made it look like do-not-commit code

@lewing
Copy link
Member Author

lewing commented Oct 30, 2020

It isn't really a sample, it is a runtime test see #44016

Makes sense, the 1 second sleep made it look like do-not-commit code

Delay is in milliseconds and it has to be > 0 or it will not actually trigger the bug it is testing.

@lewing lewing requested a review from kg October 30, 2020 05:12
@lewing lewing force-pushed the wasm-underlying-entrypoint branch from a7fc3d8 to 9c13e09 Compare October 30, 2020 05:47
@akoeplinger
Copy link
Member

Please update https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/Common/tests/WasmTestRunner/WasmTestRunner.cs as well.

You can also remove the internal code for ThreadPool.PumpThreadPool() and TimerQueue.PumpTimerQueue(), as well as mono_wasm_pump_threadpool.

@lewing
Copy link
Member Author

lewing commented Oct 30, 2020 •
edited
Loading

Please update https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/Common/tests/WasmTestRunner/WasmTestRunner.cs as well.

You can also remove the internal code for ThreadPool.PumpThreadPool() and TimerQueue.PumpTimerQueue(), as well as mono_wasm_pump_threadpool.

This change doen't change the test runner see #44046

@monojenkins monojenkins mentioned this pull request Oct 30, 2020
Merged
This was referenced Oct 30, 2020
Closed
Closed
@lewing
Copy link
Member Author

lewing commented Oct 30, 2020

The runtime tests are failing because the dependency on System.Private.Runtime.InteropServices.JavaScript isn't getting tracked correctly and this change actually uses it. Looking into it more.

@kjpou1 @lewing
[browser][bindings] Add support for calling the async Task entry point.
11bfaa6 
- When the entry point of an assembly is async we need to execute that method and return the Task object
- The Task object that is return from the execution of the async entry point will be marshaled to a JavaScript Promise.
@lewing lewing force-pushed the wasm-underlying-entrypoint branch from 5ebf69c to 531b1fc Compare November 3, 2020 01:26
kjpou1
kjpou1 approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@kjpou1 kjpou1 left a comment

Choose a reason for hiding this comment

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

This looks good

@runfoapp runfoapp bot mentioned this pull request Nov 3, 2020
Closed
@lewing
Copy link
Member Author

lewing commented Nov 3, 2020

browser test failure is #44214 which is a test that isn't meant to run on Browser

@lewing lewing dismissed kg’s stale review November 4, 2020 05:58

issues were addressed

@lewing lewing merged commit 4730697 into dotnet:master Nov 4, 2020
akoeplinger pushed a commit to dotnet/xharness that referenced this pull request Nov 5, 2020
@lewing
Wasm make *.Run async (#345)
ab2eee6 
Make the Run in the threadless runner and WasmApplicationEntry point async so that it can yield back to js to resolve promises correctly.  Depends on dotnet/runtime#44045, helps with dotnet/runtime#43958
monojenkins pushed a commit to monojenkins/mono that referenced this pull request Nov 5, 2020
@lewing
Use the new async Run method from xharness and implement a test
27b76cb 
Update browser tests to use dotnet/xharness#345 and be able to run promise related tests. Depends on dotnet/runtime#44045

The build will fail until the xharness change makes it in.
akoeplinger pushed a commit to mono/mono that referenced this pull request Nov 6, 2020
@monojenkins @lewing
Use the new async Run method from xharness and implement a test (#20534)
6bbc48d 
Update browser tests to use dotnet/xharness#345 and be able to run promise related tests. Depends on dotnet/runtime#44045

The build will fail until the xharness change makes it in.

Co-authored-by: lewing <lewing@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CoffeeFlux CoffeeFlux CoffeeFlux left review comments

@kjpou1 kjpou1 kjpou1 approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@steveisok steveisok Awaiting requested review from steveisok

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@EgorBo EgorBo Awaiting requested review from EgorBo

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

@kg kg Awaiting requested review from kg

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-VM-meta-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lewing @kg @akoeplinger @kjpou1 @CoffeeFlux