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

[browser] js string marshaling by value #95180

Merged
merged 7 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/Common/src/Interop/Browser/Interop.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@ internal static unsafe partial class Runtime
public static extern unsafe void BindCSFunction(in string fully_qualified_name, int signature_hash, void* signature, out int is_exception, out object result);
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern void ResolveOrRejectPromise(void* data);

#if !ENABLE_JS_INTEROP_BY_VALUE
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern IntPtr RegisterGCRoot(IntPtr start, int bytesSize, IntPtr name);
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern void DeregisterGCRoot(IntPtr handle);
#endif

#if FEATURE_WASM_THREADS
[MethodImpl(MethodImplOptions.InternalCall)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' != 'browser'">SR.SystemRuntimeInteropServicesJavaScript_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
<FeatureWasmThreads Condition="'$(TargetPlatformIdentifier)' == 'browser' and '$(MonoWasmBuildVariant)' == 'multithread'">true</FeatureWasmThreads>
<WasmEnableJsInteropByValue Condition="'$(TargetPlatformIdentifier)' == 'browser' and '$(WasmEnableJsInteropByValue)' == '' and '$(FeatureWasmThreads)' == 'true'">true</WasmEnableJsInteropByValue>
<WasmEnableJsInteropByValue Condition="'$(TargetPlatformIdentifier)' == 'browser' and '$(WasmEnableJsInteropByValue)' == ''">false</WasmEnableJsInteropByValue>
<WasmEnableLegacyJsInterop Condition="'$(TargetPlatformIdentifier)' == 'browser' and '$(WasmEnableLegacyJsInterop)' == ''">true</WasmEnableLegacyJsInterop>
<DefineConstants Condition="'$(FeatureWasmThreads)' == 'true'" >$(DefineConstants);FEATURE_WASM_THREADS</DefineConstants>
<DefineConstants Condition="'$(WasmEnableLegacyJsInterop)' == 'false'" >$(DefineConstants);DISABLE_LEGACY_JS_INTEROP</DefineConstants>
<DefineConstants Condition="'$(WasmEnableJsInteropByValue)' == 'true'" >$(DefineConstants);ENABLE_JS_INTEROP_BY_VALUE</DefineConstants>
<EmitCompilerGeneratedFiles Condition="'$(Configuration)' == 'Debug' and '$(TargetPlatformIdentifier)' == 'browser'">true</EmitCompilerGeneratedFiles>
</PropertyGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,9 @@ public unsafe void ToManaged(out object?[]? value)
arg.ToManaged(out val);
value[i] = val;
}
#if !ENABLE_JS_INTEROP_BY_VALUE
Interop.Runtime.DeregisterGCRoot(slot.IntPtrValue);
#endif
Marshal.FreeHGlobal(slot.IntPtrValue);
}

Expand All @@ -366,7 +368,9 @@ public unsafe void ToJS(object?[] value)
slot.Type = MarshalerType.Array;
JSMarshalerArgument* payload = (JSMarshalerArgument*)Marshal.AllocHGlobal(bytes);
Unsafe.InitBlock(payload, 0, (uint)bytes);
#if !ENABLE_JS_INTEROP_BY_VALUE
Interop.Runtime.RegisterGCRoot((IntPtr)payload, bytes, IntPtr.Zero);
#endif
for (int i = 0; i < slot.Length; i++)
{
ref JSMarshalerArgument arg = ref payload[i];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@ public unsafe void ToManaged(out string? value)
value = null;
return;
}

#if ENABLE_JS_INTEROP_BY_VALUE
value = Marshal.PtrToStringUni(slot.IntPtrValue, slot.Length);
Marshal.FreeHGlobal(slot.IntPtrValue);
#else
fixed (void* argAsRoot = &slot.IntPtrValue)
{
value = Unsafe.AsRef<string>(argAsRoot);
}
#endif
}

/// <summary>
Expand All @@ -42,6 +46,10 @@ public unsafe void ToJS(string? value)
else
{
slot.Type = MarshalerType.String;
#if ENABLE_JS_INTEROP_BY_VALUE
slot.IntPtrValue = Marshal.StringToHGlobalUni(value); // alloc, JS side will free
slot.Length = value.Length;
#else
// here we treat JSMarshalerArgument.IntPtrValue as root, because it's allocated on stack
// or we register the buffer with JSFunctionBinding._RegisterGCRoot
// We assume that GC would keep updating on GC move
Expand All @@ -52,6 +60,7 @@ public unsafe void ToJS(string? value)
var currentRoot = (IntPtr*)Unsafe.AsPointer(ref cpy);
argAsRoot[0] = currentRoot[0];
}
#endif
}
}

Expand All @@ -78,7 +87,9 @@ public unsafe void ToManaged(out string?[]? value)
arg.ToManaged(out val);
value[i] = val;
}
#if !ENABLE_JS_INTEROP_BY_VALUE
Interop.Runtime.DeregisterGCRoot(slot.IntPtrValue);
#endif
Marshal.FreeHGlobal(slot.IntPtrValue);
}

Expand All @@ -100,7 +111,9 @@ public unsafe void ToJS(string?[] value)
slot.Type = MarshalerType.Array;
JSMarshalerArgument* payload = (JSMarshalerArgument*)Marshal.AllocHGlobal(bytes);
Unsafe.InitBlock(payload, 0, (uint)bytes);
#if !ENABLE_JS_INTEROP_BY_VALUE
Interop.Runtime.RegisterGCRoot((IntPtr)payload, bytes, IntPtr.Zero);
#endif
for (int i = 0; i < slot.Length; i++)
{
ref JSMarshalerArgument arg = ref payload[i];
Expand Down
3 changes: 3 additions & 0 deletions src/mono/wasm/build/WasmApp.Native.targets
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@
<_EmccCFlags Include="-DENABLE_AOT_PROFILER=1" Condition="$(WasmProfilers.Contains('aot'))" />
<_EmccCFlags Include="-DENABLE_BROWSER_PROFILER=1" Condition="$(WasmProfilers.Contains('browser'))" />
<_EmccCFlags Include="-DDISABLE_LEGACY_JS_INTEROP=1" Condition="'$(WasmEnableLegacyJsInterop)' == 'false'" />
<_EmccCFlags Include="-DENABLE_JS_INTEROP_BY_VALUE=1" Condition="'$(WasmEnableJsInteropByValue)' == 'true'" />

<_EmccCFlags Include="-DGEN_PINVOKE=1" />
<_EmccCFlags Include="-emit-llvm" />
Expand Down Expand Up @@ -288,6 +289,8 @@
<EmscriptenEnvVars Include="EM_FROZEN_CACHE=True" Condition="'$(WasmCachePath)' == '$(EmscriptenCacheSdkCacheDir)'" />
<EmscriptenEnvVars Include="DISABLE_LEGACY_JS_INTEROP=1" Condition="'$(WasmEnableLegacyJsInterop)' == 'false'" />
<EmscriptenEnvVars Include="DISABLE_LEGACY_JS_INTEROP=0" Condition="'$(WasmEnableLegacyJsInterop)' != 'false'" />
<EmscriptenEnvVars Include="ENABLE_JS_INTEROP_BY_VALUE=1" Condition="'$(WasmEnableJsInteropByValue)' == 'true'" />
<EmscriptenEnvVars Include="ENABLE_JS_INTEROP_BY_VALUE=0" Condition="'$(WasmEnableJsInteropByValue)' != 'true'" />
<EmscriptenEnvVars Include="WASM_ENABLE_SIMD=1" Condition="'$(WasmEnableSIMD)' != 'false'" />
<EmscriptenEnvVars Include="WASM_ENABLE_SIMD=0" Condition="'$(WasmEnableSIMD)' == 'false'" />
<EmscriptenEnvVars Include="WASM_ENABLE_EH=1" Condition="'$(WasmEnableExceptionHandling)' != 'false'" />
Expand Down
3 changes: 3 additions & 0 deletions src/mono/wasm/build/WasmApp.targets
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
Defaults to false.
- $(WasmAotProfilePath) - Path to an AOT profile file.
- $(WasmEnableLegacyJsInterop) - Include support for legacy JS interop. Defaults to true.
- $(WasmEnableJsInteropByValue) - Make JS interop to pass string by value instead of by reference. Defaults to false. Defaults to true for build with threads.
- $(WasmEnableExceptionHandling) - Enable support for the WASM post MVP Exception Handling runtime extension.
- $(WasmEnableSIMD) - Enable support for the WASM post MVP SIMD runtime extension.
- $(WasmEnableWebcil) - Enable conversion of assembly .dlls to Webcil wrapped in .wasm (default: true)
Expand Down Expand Up @@ -115,6 +116,8 @@
<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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@pavelsavara pavelsavara Nov 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pavelsavara pavelsavara Nov 29, 2023

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

<WasmEnableJsInteropByValue Condition="'$(WasmEnableJsInteropByValue)' == ''">false</WasmEnableJsInteropByValue>

<!--<WasmStripAOTAssemblies Condition="'$(AOTMode)' == 'LLVMOnlyInterp'">false</WasmStripAOTAssemblies>-->
<!--<WasmStripAOTAssemblies Condition="'$(WasmStripAOTAssemblies)' == ''">$(RunAOTCompilation)</WasmStripAOTAssemblies>-->
Expand Down
1 change: 1 addition & 0 deletions src/mono/wasm/runtime/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ project(mono-wasm-runtime C)

option(DISABLE_THREADS "defined if the build does NOT support multithreading" ON)
option(DISABLE_LEGACY_JS_INTEROP "defined if the build does not support legacy JavaScript interop" OFF)
option(ENABLE_JS_INTEROP_BY_VALUE "defined when JS interop without pointers to managed objects" OFF)

set(CMAKE_EXECUTABLE_SUFFIX ".js")
add_executable(dotnet.native corebindings.c driver.c pinvoke.c)
Expand Down
3 changes: 3 additions & 0 deletions src/mono/wasm/runtime/corebindings.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,11 @@ void bindings_initialize_internals (void)
mono_add_internal_call ("Interop/Runtime::InvokeJSImport", mono_wasm_invoke_js_import);
mono_add_internal_call ("Interop/Runtime::BindCSFunction", mono_wasm_bind_cs_function);
mono_add_internal_call ("Interop/Runtime::ResolveOrRejectPromise", mono_wasm_resolve_or_reject_promise);

#ifndef ENABLE_JS_INTEROP_BY_VALUE
mono_add_internal_call ("Interop/Runtime::RegisterGCRoot", mono_wasm_register_root);
mono_add_internal_call ("Interop/Runtime::DeregisterGCRoot", mono_wasm_deregister_root);
#endif /* ENABLE_JS_INTEROP_BY_VALUE */

#ifndef DISABLE_THREADS
mono_add_internal_call ("Interop/Runtime::InstallWebWorkerInterop", mono_wasm_install_js_worker_interop);
Expand Down
31 changes: 22 additions & 9 deletions src/mono/wasm/runtime/marshal-to-cs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import MonoWasmThreads from "consts:monoWasmThreads";
import BuildConfiguration from "consts:configuration";
import WasmEnableJsInteropByValue from "consts:wasmEnableJsInteropByValue";

import { isThenable } from "./cancelable-promise";
import cwraps from "./cwraps";
Expand All @@ -18,7 +19,7 @@ import {
} from "./marshal";
import { get_marshaler_to_js_by_type } from "./marshal-to-js";
import { _zero_region, localHeapViewF64, localHeapViewI32, localHeapViewU8 } from "./memory";
import { stringToMonoStringRoot } from "./strings";
import { stringToMonoStringRoot, stringToUTF16 } from "./strings";
import { JSMarshalerArgument, JSMarshalerArguments, JSMarshalerType, MarshalerToCs, MarshalerToJs, BoundMarshalerToCs, MarshalerType } from "./types/internal";
import { TypedArray } from "./types/emscripten";
import { addUnsettledPromise, settleUnsettledPromise } from "./pthreads/shared/eventloop";
Expand Down Expand Up @@ -229,12 +230,20 @@ function _marshal_string_to_cs(arg: JSMarshalerArgument, value: string) {
}

function _marshal_string_to_cs_impl(arg: JSMarshalerArgument, value: string) {
const root = get_string_root(arg);
try {
stringToMonoStringRoot(value, root);
}
finally {
root.release();
if (WasmEnableJsInteropByValue) {
const bufferLen = value.length * 2;
const buffer = Module._malloc(bufferLen);
stringToUTF16(buffer as any, buffer as any + bufferLen, value);
set_arg_intptr(arg, buffer);
set_arg_length(arg, value.length);
} else {
const root = get_string_root(arg);
try {
stringToMonoStringRoot(value, root);
}
finally {
root.release();
}
}
}

Expand Down Expand Up @@ -505,7 +514,9 @@ export function marshal_array_to_cs_impl(arg: JSMarshalerArgument, value: Array<
if (element_type == MarshalerType.String) {
mono_check(Array.isArray(value), "Value is not an Array");
_zero_region(buffer_ptr, buffer_length);
cwraps.mono_wasm_register_root(buffer_ptr, buffer_length, "marshal_array_to_cs");
if (!WasmEnableJsInteropByValue) {
cwraps.mono_wasm_register_root(buffer_ptr, buffer_length, "marshal_array_to_cs");
}
for (let index = 0; index < length; index++) {
const element_arg = get_arg(<any>buffer_ptr, index);
_marshal_string_to_cs(element_arg, value[index]);
Expand All @@ -514,7 +525,9 @@ export function marshal_array_to_cs_impl(arg: JSMarshalerArgument, value: Array<
else if (element_type == MarshalerType.Object) {
mono_check(Array.isArray(value), "Value is not an Array");
_zero_region(buffer_ptr, buffer_length);
cwraps.mono_wasm_register_root(buffer_ptr, buffer_length, "marshal_array_to_cs");
if (!WasmEnableJsInteropByValue) {
cwraps.mono_wasm_register_root(buffer_ptr, buffer_length, "marshal_array_to_cs");
}
for (let index = 0; index < length; index++) {
const element_arg = get_arg(<any>buffer_ptr, index);
_marshal_cs_object_to_cs(element_arg, value[index]);
Expand Down
30 changes: 22 additions & 8 deletions src/mono/wasm/runtime/marshal-to-js.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import MonoWasmThreads from "consts:monoWasmThreads";
import BuildConfiguration from "consts:configuration";
import WasmEnableJsInteropByValue from "consts:wasmEnableJsInteropByValue";

import cwraps from "./cwraps";
import { _lookup_js_owned_object, mono_wasm_get_jsobj_from_js_handle, mono_wasm_release_cs_owned_object, register_with_jsv_handle, setup_managed_proxy, teardown_managed_proxy } from "./gc-handles";
Expand All @@ -15,7 +16,7 @@ import {
get_signature_res_type, get_arg_u16, array_element_size, get_string_root,
ArraySegment, Span, MemoryViewType, get_signature_arg3_type, get_arg_i64_big, get_arg_intptr, get_arg_element_type, JavaScriptMarshalerArgSize, proxy_debug_symbol
} from "./marshal";
import { monoStringToString } from "./strings";
import { monoStringToString, utf16ToString } from "./strings";
import { GCHandleNull, JSMarshalerArgument, JSMarshalerArguments, JSMarshalerType, MarshalerToCs, MarshalerToJs, BoundMarshalerToJs, MarshalerType } from "./types/internal";
import { TypedArray } from "./types/emscripten";
import { get_marshaler_to_cs_by_type, jsinteropDoc, marshal_exception_to_cs } from "./marshal-to-cs";
Expand Down Expand Up @@ -310,12 +311,21 @@ export function marshal_string_to_js(arg: JSMarshalerArgument): string | null {
if (type == MarshalerType.None) {
return null;
}
const root = get_string_root(arg);
try {
const value = monoStringToString(root);
if (WasmEnableJsInteropByValue) {
const buffer = get_arg_intptr(arg);
const len = get_arg_length(arg) * 2;
const value = utf16ToString(<any>buffer, <any>buffer + len);
Module._free(buffer as any);
return value;
} finally {
root.release();
}
else {
const root = get_string_root(arg);
try {
const value = monoStringToString(root);
return value;
} finally {
root.release();
}
}
}

Expand Down Expand Up @@ -421,15 +431,19 @@ function _marshal_array_to_js_impl(arg: JSMarshalerArgument, element_type: Marsh
const element_arg = get_arg(<any>buffer_ptr, index);
result[index] = marshal_string_to_js(element_arg);
}
cwraps.mono_wasm_deregister_root(<any>buffer_ptr);
if (!WasmEnableJsInteropByValue) {
cwraps.mono_wasm_deregister_root(<any>buffer_ptr);
}
}
else if (element_type == MarshalerType.Object) {
result = new Array(length);
for (let index = 0; index < length; index++) {
const element_arg = get_arg(<any>buffer_ptr, index);
result[index] = _marshal_cs_object_to_js(element_arg);
}
cwraps.mono_wasm_deregister_root(<any>buffer_ptr);
if (!WasmEnableJsInteropByValue) {
cwraps.mono_wasm_deregister_root(<any>buffer_ptr);
}
}
else if (element_type == MarshalerType.JSObject) {
result = new Array(length);
Expand Down
2 changes: 2 additions & 0 deletions src/mono/wasm/runtime/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const monoWasmThreads = process.env.MonoWasmThreads === "true" ? true : false;
const wasmEnableSIMD = process.env.WASM_ENABLE_SIMD === "1" ? true : false;
const wasmEnableExceptionHandling = process.env.WASM_ENABLE_EH === "1" ? true : false;
const wasmEnableLegacyJsInterop = process.env.DISABLE_LEGACY_JS_INTEROP !== "1" ? true : false;
const wasmEnableJsInteropByValue = process.env.ENABLE_JS_INTEROP_BY_VALUE == "1" ? true : false;
const monoDiagnosticsMock = process.env.MonoDiagnosticsMock === "true" ? true : false;
// because of stack walk at src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
// and unit test at src\libraries\System.Runtime.InteropServices.JavaScript\tests\System.Runtime.InteropServices.JavaScript.Legacy.UnitTests\timers.mjs
Expand Down Expand Up @@ -103,6 +104,7 @@ const envConstants = {
monoDiagnosticsMock,
gitHash,
wasmEnableLegacyJsInterop,
wasmEnableJsInteropByValue,
isContinuousIntegrationBuild,
};

Expand Down
3 changes: 3 additions & 0 deletions src/mono/wasm/runtime/wasm-config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@
/* Support for legacy JS interop is disabled */
#cmakedefine DISABLE_LEGACY_JS_INTEROP

/* Support for JS interop without pointers to managed objects */
#cmakedefine ENABLE_JS_INTEROP_BY_VALUE

#endif/*__MONO_WASM_CONFIG_H__*/
Loading
Loading