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

[mono] merge wasm-threading-eventpipe into main #68232

Merged
merged 139 commits into from
Apr 26, 2022

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Apr 19, 2022

Merge initial work on multi-threaded WebAssembly.

The normal wasm build is single-threaded. There should be no functional changes to its behavior.

To enable a multi-threaded build pass /p:WasmEnableThreads=true. See src/mono/wasm/threads.md for details.

The big changes are:

  1. The normal ref assemblies related to threading retain [UnsupportedOSPlatform("browser")] attributes for various threading-related functions
  2. In System.Private.CoreLib, the [UnsupportedOSPlatform] attributes are removed, and functions that used to always throw PNSE nwo do a runtime check using System.Threading.Thread.IsThreadStartSupported to check if threading is enabled in the current build.
  3. A new nuget Microsoft.NET.WebAssembly.Threading is created. It contains experimental ref assemblies without the [UnsupportedOSPlatform] attributes. The intention is that code opting into experimenting with multithreading will include this nuget by setting some property that will be used by the wasm MSBuild SDK to pick a multi-threaded runtime build and configure things appropriately. (The SDK updates don't exist yet).
  4. In the multi-threaded runtime we don't use Emscripten's "main thread" option (ie: the browser thread is the main one); we also continue to run certain runtime-internal jobs (finalizers, GC pump) on the main thread even in the multi-threaded runtime.

Remaining work is tracked in the related issue #68162

radekdoulik and others added 30 commits December 7, 2021 21:46
Co-authored-by: Zoltan Varga <vargaz@gmail.com>
    src/mono/mono/mini/mini-runtime.c:3407:25: error: ‘invoke’ undeclared (first use in this function); did you mean ‘revoke’?
       3407 |                         invoke = mono_marshal_get_runtime_invoke_dynamic ();
Environment setting https://github.com/emscripten-core/emscripten/blob/2.0.34/src/settings.js#L616-L641

From emscripten 2.0.25 release notes

    - Support for the 'shell' environment is now disabled by default.  Running under
      `d8`, `js`, or `jsc` is not something that most emscripten users ever want to
      do, so including the support code is, more often than not, unnecessary.  Users
      who want shell support can enable it by including 'shell' in `-s ENVIRONMENT`
      (dotnet#14535).

Example of the the size increase for bench sample:

    -a---          12/10/2021  3:35 PM         382113 dotnet.js
    -a---          12/13/2021 10:37 AM         383589 dotnet.js
To enable, pass `/p:WasmEnableThreads` when building the runtime

./build.sh -Subset mono+libs -os Browser -arch wasm /p:WasmEnableThreads=true
… that worker initialization can find dotnet.js
…roperly get a Module instance, since we broke the API
Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

Great job putting this together!

@lambdageek
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Infrastructure changes LGTM. Good work 👍

@lambdageek
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -10,9 +10,9 @@ if(ENABLE_PERFTRACING)
add_definitions(-DENABLE_PERFTRACING_PAL_TCP)
endif (FEATURE_PERFTRACING_PAL_TCP)

if (FEATURE_PERFTRACING_DISABLE_LISTEN_PORTS)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Member Author

@lambdageek lambdageek Apr 26, 2022

Choose a reason for hiding this comment

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

this is a fix for a typo, the old cmake variable name was unused


## Building ##

Build with `/p:WasmEnableThreads=true` to enable support for multi-threading.
Copy link
Member

Choose a reason for hiding this comment

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

Will this also enable EventPipe diagnostics support?

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 so. Once we have full user-visible multithreading there's no particular reason to exclude eventpipe.

@lambdageek lambdageek merged commit b44c008 into dotnet:main Apr 26, 2022
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

LGTM thank you! Found just one small formatting nit.

#endif
#endif

internal static void ThrowIfNoThreadStart() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting: curly brace on new line

Copy link
Member Author

Choose a reason for hiding this comment

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

@lewing
Copy link
Member

lewing commented Apr 26, 2022

🎊

@ghost ghost locked as resolved and limited conversation to collaborators May 27, 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.