[browser] js string marshaling by value #95180

pavelsavara · 2023-11-23T15:57:43Z

New MSbuild flag WasmJsInteropByValue which is false by default on ST build and true on MT build.

Make string marshaling by value rather than by reference to MonoString*

The motivation is that

Native AOT is not Mono
MT may need to marshal to unmanaged thread

This removes optimization we have for interned strings, when enabled.

I didn't fix it for legacy interop.

ghost · 2023-11-23T15:57:51Z

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

Issue Details

Make string marshaling by value rather than by reference to MonoString*

The motivation is that

Native AOT is not Mono
MT may need to marshal to unmanaged thread

Author:	pavelsavara
Assignees:	pavelsavara
Labels:	`arch-wasm`, `area-System.Runtime.InteropServices.JavaScript`, `os-browser`
Milestone:	9.0.0

pavelsavara · 2023-11-23T16:24:22Z

/azp run runtime-wasm

azure-pipelines · 2023-11-23T16:24:31Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-11-23T17:32:10Z

/azp run runtime-wasm

azure-pipelines · 2023-11-23T17:32:24Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-11-23T17:36:20Z

/azp run runtime-wasm

azure-pipelines · 2023-11-23T17:36:30Z

Azure Pipelines successfully started running 1 pipeline(s).

kg

I'm not sure I understand why we're doing this, but the code looks fine

src/mono/wasm/wasm.proj

pavelsavara · 2023-11-27T10:01:33Z

I'm not sure I understand why we're doing this

@kg

For NativeAOT sake - there is no Mono and MonoString* there.
for deputy MT experiment, dealing with MonoString requires we enter MONO_ENTER_GC_UNSAFE on UI thread, which could be blocking operation.

# Conflicts: # src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj

pavelsavara · 2023-11-27T14:53:12Z

/azp run runtime-wasm

azure-pipelines · 2023-11-27T14:53:22Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-11-28T12:36:14Z

/azp run runtime-wasm

azure-pipelines · 2023-11-28T12:36:23Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-11-28T13:50:09Z

/azp run runtime-wasm

azure-pipelines · 2023-11-28T13:50:20Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-11-28T17:15:44Z

/azp run runtime-wasm

azure-pipelines · 2023-11-28T17:15:54Z

Azure Pipelines successfully started running 1 pipeline(s).

radical · 2023-11-28T18:49:31Z

src/mono/wasm/build/WasmApp.targets

    <WasmEnableExceptionHandling Condition="'$(WasmEnableExceptionHandling)' == ''">true</WasmEnableExceptionHandling>
    <WasmEnableSIMD Condition="'$(WasmEnableSIMD)' == ''">$(WasmEnableExceptionHandling)</WasmEnableSIMD>
    <WasmEnableLegacyJsInterop Condition="'$(WasmEnableLegacyJsInterop)' == ''">true</WasmEnableLegacyJsInterop>
+    <WasmEnableJsInteropByValue Condition="'$(WasmEnableJsInteropByValue)' == '' and ( '$(WasmEnableThreads)' == 'true' or '$(MonoWasmBuildVariant)' == 'multithread' )">true</WasmEnableJsInteropByValue>


Isn't WasmEnableThreads user facing, and MonoWasmBuildVariant is meant more for the wasm/runtime build itself? I see that it is being used in a couple of other places, which we should fix too. Not necessary for this PR.

This makes it easy to work in-tree with samples and unit tests when you just have MonoWasmBuildVariant set globally and sometimes native rebuild kicks in. Maybe it could be unified somehow, I'm not 100% sure about it.

You can set WasmEnableThreads based on MonoWasmBuildVariant in WasmApp.InTree.targets.

I would prefer if you could help me to improve it in different PR. But I would like to ask details in meantime

what are you supposed to use WasmEnableThreads or MonoWasmBuildVariant inside of

wasm.proj

WasmApp.Native.targets

C code like drive.c triggered by

runtime build (wasm.proj)

external application native rebuild (WasmApp.Native.targets)

in-tree sample native rebuild (WasmApp.InTree.targets)

inside .csproj of libraries.

inside .csproj of libraries unit tests

inside .csproj of in-tree samples

in .csproj of WBT

What are you supposed to use WasmEnableThreads or MonoWasmBuildVariant on commandline ?

when you build runtime and libraries with build.sh

when you run unit tests. This could trigger re-build of C# libraries

when you run samples in-tree. This could trigger re-build of C# libraries

How does it propagate to the nested build/publish ?
Does build on helix changes anything about it ?
Does WBT runtime installation in artifacts changes anything about it ?
Is there really benefit of having 2 of them ?

MonoWasmBuildVariant is really only for non-user projects, so wasm.proj would be an example.
All the other test cases (except WBT) use InTree targets, so adding overrides there would apply to all of them.
WBT strictly uses what the user gets, so no InTree targets.

Is there really benefit of having 2 of them ?

Not really. I think we had that before for multithread, and perftrace builds but now we don't need a string property. It is clearer to have them separate but I wouldn't mind using only WasmEnableThreads either.
What I do want to avoid is using internal build properties in user facing targets.

cc @lambdageek

pavelsavara · 2023-11-28T23:59:09Z

/azp run runtime-wasm

azure-pipelines · 2023-11-28T23:59:21Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Nov 23, 2023

pavelsavara added this to the 9.0.0 milestone Nov 23, 2023

pavelsavara requested a review from maraf November 23, 2023 15:57

pavelsavara self-assigned this Nov 23, 2023

pavelsavara changed the title ~~[browser] Configurable string marshling by value~~ [browser] js string marshling by value Nov 23, 2023

pavelsavara changed the title ~~[browser] js string marshling by value~~ [browser] js string marshaling by value Nov 23, 2023

pavelsavara marked this pull request as ready for review November 23, 2023 16:26

pavelsavara requested review from kg and lewing as code owners November 23, 2023 16:26

build-analysis bot mentioned this pull request Nov 23, 2023

[wasm] DebuggerTests.MiscTests.DebuggerTests.MiscTests.TestDebugUsingMultiThreadedRuntime failing #89909

Closed

kg approved these changes Nov 24, 2023

View reviewed changes

maraf reviewed Nov 24, 2023

View reviewed changes

src/mono/wasm/wasm.proj Outdated Show resolved Hide resolved

rebase

4a2bf17

pavelsavara force-pushed the browser_string_interop_by_value branch from c66f1b0 to 4a2bf17 Compare November 27, 2023 09:58

maraf approved these changes Nov 27, 2023

View reviewed changes

build-analysis bot mentioned this pull request Nov 27, 2023

[wasm] Playwright failing #94240

Closed

pavelsavara added 2 commits November 27, 2023 15:51

fix

923f4b2

Merge branch 'main' into browser_string_interop_by_value

cd15e29

# Conflicts: # src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj

fix

4882659

native

40016d2

pavelsavara requested a review from radical as a code owner November 28, 2023 13:48

fix

cb70255

radical reviewed Nov 28, 2023

View reviewed changes

fix

30ad663

build-analysis bot mentioned this pull request Nov 29, 2023

Tracking issue for CI build timeouts #76454

Closed

pavelsavara merged commit c089092 into dotnet:main Nov 29, 2023

pavelsavara deleted the browser_string_interop_by_value branch November 29, 2023 09:45

pavelsavara mentioned this pull request Dec 12, 2023

[browser][MT] Multithreading and JavaScript async interop in .NET 9 #85592

Closed

31 tasks

maraf mentioned this pull request Dec 19, 2023

[NativeAOT LLVM] Enable JS interop dotnet/runtimelab#2440

Merged

github-actions bot locked and limited conversation to collaborators Dec 30, 2023

[browser] js string marshaling by value #95180

[browser] js string marshaling by value #95180

Uh oh!

Conversation

pavelsavara commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 23, 2023

Uh oh!

pavelsavara commented Nov 23, 2023

Uh oh!

azure-pipelines bot commented Nov 23, 2023

Uh oh!

pavelsavara commented Nov 23, 2023

Uh oh!

azure-pipelines bot commented Nov 23, 2023

Uh oh!

pavelsavara commented Nov 23, 2023

Uh oh!

azure-pipelines bot commented Nov 23, 2023

Uh oh!

kg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pavelsavara commented Nov 27, 2023

Uh oh!

pavelsavara commented Nov 27, 2023

Uh oh!

azure-pipelines bot commented Nov 27, 2023

Uh oh!

pavelsavara commented Nov 28, 2023

Uh oh!

azure-pipelines bot commented Nov 28, 2023

Uh oh!

pavelsavara commented Nov 28, 2023

Uh oh!

azure-pipelines bot commented Nov 28, 2023

Uh oh!

pavelsavara commented Nov 28, 2023

Uh oh!

azure-pipelines bot commented Nov 28, 2023

Uh oh!

radical Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

pavelsavara Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radical Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

pavelsavara Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radical Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

pavelsavara commented Nov 28, 2023

Uh oh!

azure-pipelines bot commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pavelsavara commented Nov 23, 2023 •

edited

Loading

pavelsavara Nov 28, 2023 •

edited

Loading

pavelsavara Nov 29, 2023 •

edited

Loading