[SUPER DRAFT] New API for invoking JS functions from C# #60765

kg · 2021-10-22T14:19:13Z

This PR (which relies on the custom marshalers PR, #47640) introduces a new icall (InvokeJSFunction) that is used to invoke JS functions from C#. It takes a 'do it yourself' approach to things like error handling (in order to avoid JS roundtrips for try/catch blocks) and attempts to remove overhead in various other places so that scenarios not needing that overhead can be much faster. It's also designed to be (partially) AOT friendly by making the core icall not generic.

The icall enforces some rules to produce good performance:

The name of the function being invoked must be interned. This allows the function itself to be cached using the address of the interned string as a key (interned strings are not relocatable), so the lookup is very fast.
The function pointed to must not change at runtime (so it can be cached).
The function being invoked must not return a value, if it does the icall will return an error code.
- This is because marshaling a return value of unknown type means we have to box it - JS->C# invokes already pay this unavoidable tax and it sucks.
The function being invoked must not throw an Error, if it does the icall will return an error code.
- Marshaling exceptions universally is finicky and in some scenarios the caller wants to handle the error in their own way, i.e. set a flag or store info on the error somewhere instead of throwing a C# exception.
The maximum number of arguments is 3.
All arguments are passed as (RuntimeTypeHandle type, IntPtr arg) pairs, where (depending on type):
- If type is a pointer, arg is the raw pointer value.
- If type is a ValueType, arg is the address of the value.
- Otherwise (if type is a reference type), arg is the address of the location of the object reference on the stack. This is so that if the GC relocates the object, the callee will see the new post-relocation address.

The new icall offers some new features as part of the deal:

The function name can contain dots (i.e. Module._free) and the icall will walk the scope chain to find the target function - we can afford to do this since we cache the result.
If the function name starts with BINDING, MONO or INTERNAL, the icall will look the function up inside the appropriate runtime API object instead of in the global scope, so those function names will work even if we are running modularized and they are not exposed in the global scope.

For scenarios not covered by the new icall, the intended solution is to expose higher-level convenience wrappers that provide the bells and whistles people expect. This keeps the icall slim and fast so that in use cases where you want maximum performance, you can get it. At present the PR provides InvokeJSResult InvokeJSFunctionByName<T1, T2, T3> (string internedFunctionName, ref T1 arg1, ref T2 arg2, ref T3 arg3) as a convenience wrapper that automatically handles producing the type/address pairs for you.

As a proof of concept, the PR changes JSObject.Invoke, JSObject.SetProperty and JSObject.GetProperty to go through the new icall. I have a feeling we don't actually want to do this, but it serves as a good example of how to use the icall and was also useful for flushing out bugs and getting a sense of its performance.

dotnet-issue-labeler · 2021-10-22T14:19:18Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2021-10-22T14:33:11Z

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

Issue Details

This PR (which relies on the custom marshalers PR, #47640) introduces a new icall (InvokeJSFunction) that is used to invoke JS functions from C#. It takes a 'do it yourself' approach to things like error handling (in order to avoid JS roundtrips for try/catch blocks) and attempts to remove overhead in various other places so that scenarios not needing that overhead can be much faster. It's also designed to be (partially) AOT friendly by making the core icall not generic.

The icall enforces some rules to produce good performance:

The name of the function being invoked must be interned. This allows the function itself to be cached using the address of the interned string as a key (interned strings are not relocatable), so the lookup is very fast.
The function pointed to must not change at runtime (so it can be cached).
The function being invoked must not return a value, if it does the icall will return an error code.
- This is because marshaling a return value of unknown type means we have to box it - JS->C# invokes already pay this unavoidable tax and it sucks.
The function being invoked must not throw an Error, if it does the icall will return an error code.
- Marshaling exceptions universally is finicky and in some scenarios the caller wants to handle the error in their own way, i.e. set a flag or store info on the error somewhere instead of throwing a C# exception.
The maximum number of arguments is 3.
All arguments are passed as (RuntimeTypeHandle type, IntPtr arg) pairs, where (depending on type):
- If type is a pointer, arg is the raw pointer value.
- If type is a ValueType, arg is the address of the value.
- Otherwise (if type is a reference type), arg is the address of the location of the object reference on the stack. This is so that if the GC relocates the object, the callee will see the new post-relocation address.

The new icall offers some new features as part of the deal:

The function name can contain dots (i.e. Module._free) and the icall will walk the scope chain to find the target function - we can afford to do this since we cache the result.
If the function name starts with BINDING, MONO or INTERNAL, the icall will look the function up inside the appropriate runtime API object instead of in the global scope, so those function names will work even if we are running modularized and they are not exposed in the global scope.

For scenarios not covered by the new icall, the intended solution is to expose higher-level convenience wrappers that provide the bells and whistles people expect. This keeps the icall slim and fast so that in use cases where you want maximum performance, you can get it. At present the PR provides InvokeJSResult InvokeJSFunctionByName<T1, T2, T3> (string internedFunctionName, ref T1 arg1, ref T2 arg2, ref T3 arg3) as a convenience wrapper that automatically handles producing the type/address pairs for you.

As a proof of concept, the PR changes JSObject.Invoke, JSObject.SetProperty and JSObject.GetProperty to go through the new icall. I have a feeling we don't actually want to do this, but it serves as a good example of how to use the icall and was also useful for flushing out bugs and getting a sense of its performance.

Author:	kg
Assignees:	-
Labels:	`arch-wasm`
Milestone:	-

kg · 2021-10-22T15:52:43Z

Some rough performance measurements:

|                                                    Method |            Mean |         Error |        StdDev |          Median |
|---------------------------------------------------------- |----------------:|--------------:|--------------:|----------------:|
|                                InvokeMethodIntViaInvokeJS |  8,275,348.2 ns | 110,660.33 ns |  98,097.50 ns |  8,295,421.9 ns |
|                        InvokeMethodIntViaInvokeJSWithArgs | 15,873,066.3 ns |  85,369.67 ns |  75,677.99 ns | 15,895,607.2 ns |
|                          InvokeMethodIntViaNewInvokeIcall |  4,313,305.8 ns |  48,370.09 ns |  42,878.82 ns |  4,307,187.5 ns |

|                             InvokeMethodStringViaInvokeJS |  8,063,429.2 ns |  79,356.04 ns |  74,229.69 ns |  8,065,531.3 ns |
|                     InvokeMethodStringViaInvokeJSWithArgs | 15,287,281.0 ns | 225,844.35 ns | 200,205.13 ns | 15,225,566.6 ns |
|                       InvokeMethodStringViaNewInvokeIcall |  5,000,841.5 ns |  50,035.87 ns |  44,355.49 ns |  4,998,835.9 ns |

|                        InvokeMethodUriViaInvokeJSWithArgs | 21,299,286.7 ns | 306,285.53 ns | 255,762.29 ns | 21,227,000.1 ns |
|                          InvokeMethodUriViaNewInvokeIcall | 16,921,928.2 ns | 172,980.27 ns | 144,446.36 ns | 16,960,733.3 ns |

|                       InvokeMethodDateViaInvokeJSWithArgs | 31,449,576.9 ns | 219,972.57 ns | 183,687.06 ns | 31,444,000.1 ns |
|                         InvokeMethodDateViaNewInvokeIcall | 22,611,903.0 ns | 293,292.98 ns | 274,346.44 ns | 22,597,727.4 ns |

kg · 2021-10-24T13:06:08Z

cc @jeromelaban @SteveSandersonMS will this new invoke API satisfy your needs? My hope is that you will be able to migrate to use it under the hood and get improved performance.

pavelsavara · 2021-10-26T10:41:59Z

src/mono/wasm/runtime/roots.ts

@@ -52,6 +53,26 @@ export function mono_wasm_new_root_buffer_from_pointer(offset: VoidPtr, capacity
    return new WasmRootBuffer(offset, capacity, false, name);
 }

+/**
+ * Allocates a WasmRoot pointing to a root provided and controlled by external code.
+ * Releasing this root will not do anything, but you still need to call .release().


Could you please explain why we need this ?

External callers have object references sitting on the stack, but the GC can still move them. So we want to wrap that external stack root in a WasmRoot-api type for JS to consume. If we use a JS-owned WasmRoot instance we have to copy the ref, which is inefficient (and might have a race?)

src/mono/mono/sgen/sgen-memory-governor.c

src/mono/wasm/runtime/invoke-api.ts

pavelsavara · 2021-10-26T13:39:32Z

src/mono/wasm/runtime/invoke-api.ts

+    }
+}
+
+export function _walk_global_scope_to_find_function (str : string) : ResolveJSFunctionResult {


What are the use cases where user wants to call something on global namespace ?
Could we ask the user to pass globalThis.foo.bar() explicitly themselves ?

What are the other options we have ?

If the target was function in es6 module, how could they pass the instance wrapped somewhere deep in webpacked code ?

We already have JSObject.Invoke , do we need this one too ?

We could make globalThis mandatory, yes. Good idea.

I think JSObject.Invoke needs to be separate (but can be implemented through this), because it invokes on a this-reference (dynamic) and can throw.

src/mono/wasm/runtime/invoke-api.ts

pavelsavara · 2021-10-26T14:58:45Z

@kg could you please explain what you mean by

It's also designed to be (partially) AOT friendly by making the core icall not generic.

Also what's wrong with the existing mono_wasm_invoke_js ?
(besides that it should not have eval and live in EM_ASM_INT macro, which I both fixed in my pending PR .)

kg · 2021-10-26T16:00:41Z

@kg could you please explain what you mean by

It's also designed to be (partially) AOT friendly by making the core icall not generic.

At one point we had an invoke icall with a generic <T> return type, but that can't be AOTed (at least right now). So the current one returns object, which is inefficient (and kind of gross in terms of the burden it puts on both the caller and callee). We want strong types both for reliability and small, efficient codegen.

Also what's wrong with the existing mono_wasm_invoke_js ? (besides that it should not have eval and live in EM_ASM_INT macro, which I both fixed in my pending PR .)

I could sit down and try to carefully enumerate why I think we need a new icall, but off the top of my head (this was a long design discussion that happened quite a while ago, so it's reasonable to revisit it):

mono_wasm_invoke_js_with_args is not a tasteful API - the design does not have the traits of a good maintainable API. During the review for custom marshalers some of the same traits (weak types, boxing etc) that came up there as no-go apply to it as well.
It's hard to optimize without compatibility breaks (for example, requiring that the method name be interned), and it's easy to hit performance pitfalls by using it in an inefficient way. Taking an object[] arg basically guarantees bad performance (the only real way to optimize the allocation out requires a ThreadLocal to be safe.)
Many JS functions return undefined, and I think it's inappropriate to silently convert that to a null - it often indicates that you misunderstand the signature of the function you're calling and should be a failure.
JS functions also happily accept extra arguments and sometimes will work if arguments are missing. This API makes it easy to do that, and I think we should be explicit about argument count. (The current icall doesn't enforce a callee argument count check... I could add that on the JS side if you want but I think there are cases where the callee won't have argument count data.)
Aside from the general 'hard to optimize' problem I think making every single invoke create GC pressure is a very bad thing. We need to avoid it in the bindings.
This new icall's signature is basically a cleaned up variation on mono_wasm_invoke_js_blazor (with the exception that the callee+caller need to collaborate to handle exceptions). I think it's best to ensure our API can efficiently be used to implement blazor, uno, etc.
For mono_wasm_invoke_js, the overhead of looking up the string literal is nonzero (especially because it's not required to be interned), and not being able to pass arguments requires the caller to do unclean things like shove arguments into globals (which is not a cheap thing to do via the current API... you'd have to do SetObjectProperty per-argument.)

My endgame for this is that we use source generators to create strongly-typed wrappers for specific methods (JS pinvoke, basically) that correctly and efficiently use this new icall under the hood. Later on those wrappers could transition to using dedicated JS and bypassing the icall machinery if necessary.

kg · 2021-11-05T21:55:43Z

Now that I fixed the Iterator test, it's reliably broken with this PR applied. Still trying to figure out why.

kg · 2021-11-05T23:05:35Z

That aside, here are benchmark timings from an AOT run that just finished (the previous timings were for the interpreter):

|                                                    Method |           Mean |           Error |          StdDev |         Median |
|---------------------------------------------------------- |---------------:|----------------:|----------------:|---------------:|
|                                InvokeMethodIntViaInvokeJS |  17,569.092 us |  14,161.0743 us |  16,307.9142 us |  10,619.111 us |
|                        InvokeMethodIntViaInvokeJSWithArgs |  73,395.934 us |  58,622.9389 us |  65,159.2413 us |  64,401.000 us |
|                          InvokeMethodIntViaNewInvokeIcall |   4,045.656 us |   2,978.6066 us |   3,187.0757 us |   3,434.698 us |

|                             InvokeMethodStringViaInvokeJS |  18,148.974 us |  12,640.8114 us |  14,557.1772 us |  13,924.735 us |
|                     InvokeMethodStringViaInvokeJSWithArgs |  80,123.500 us |  69,523.8571 us |  80,063.7773 us |  36,260.125 us |
|                       InvokeMethodStringViaNewInvokeIcall |  20,558.150 us |  17,739.3901 us |  19,717.2851 us |  17,997.000 us |

|                        InvokeMethodUriViaInvokeJSWithArgs | 302,903.300 us | 246,371.2013 us | 283,721.4418 us | 117,664.000 us |
|                          InvokeMethodUriViaNewInvokeIcall |  72,661.697 us |  50,269.2145 us |  55,874.0987 us |  61,930.750 us |

|                       InvokeMethodDateViaInvokeJSWithArgs | 235,043.737 us | 198,301.3285 us | 220,411.4012 us | 113,798.001 us |
|                         InvokeMethodDateViaNewInvokeIcall |  75,834.526 us |  45,035.2109 us |  50,056.5176 us |  42,651.250 us |

Add test coverage for date marshaling Add uri test Disable some log statements Returning structs works Returning custom classes works Add null checks to describe_value Remove invalid cwrap Add trimming annotations Restore old benchmark html Reuse result and exception roots Cleaner codegen Disambiguate converters in devtools Transition back to static methods, add signature checks and more error handling Implement a simple bound method cache so that automated tests don't exhaust the scratch root buffer; make the scratch root buffer smaller Fix a LMF leak Fix C warnings Hard-coded table of marshalers is no longer needed Shorter wrapper function names for better stacks Add linker exclusions for custom marshalers Hack around bug in linker/runtime that causes System.Uri forwarder to break Rework class lookup Support passing numeric values as DateTime Transition some null returns to asserts Clean up conditionals Don't use ISO strings Change pre/post filter syntax to require an explicit "return" so it can contain multiple statements Allow ToJavaScript to accept a pointer instead of an 'in' reference Support managed pointer return/parameter types in more places Add marshal_type for pointers Add tests verifying that you can marshal structs by address and then unpack them Initial gc safety work Fix formatting rule Rename pre/post filters Fix type error in driver.c remove _pick_result_chara_for_marshal_type Add library build descriptor to ensure marshalers are not stripped when the BCL is trimmed Attempt to fix the linker stripping test code Better version of no-configured-marshaler warning Annotate SetupJSContinuation Annotate safehandle APIs and add null checks Update targets file to pass custom marshaler msbuild items through to the helix proxy project (requires another change to work) Add tests for Task and ValueTask marshaling Fix unboxing for generic structs Rebase cleanups Correct datetime test to use UTC for comparison Eliminate use of MONO. and BINDING. in closures Optimize out a js->c call Normalize some APIs to take MonoType instead of MonoClass Repair merge damage Address PR feedback Move some types around Repair merge damage Type system fixes Rework create_named_function so that it can handle larger numbers of closure keys more efficiently Remove unnecessary test instrumentation Fix closure variables being generated in the wrong place Use a single memory slab for temp_malloc Checkpoint span support Fix unbuffered filters, support ReadOnlySpan Fix auto signatures for primitives and add test Use 4-chara unicode escapes since the x escapes are not officially permitted in JSON Checkpoint C# implementation of converter generator Align everything by 8 when constructing argument buffers because if you don't do that, the runtime passes corrupt data to C# functions Don't shove raw mono pointers into root buffers since we weren't doing it before (it might be nice to do it though) Fully transition over to having C# generate signature converters C# implementation of bind_method codegen Clean up the bindings named closure table management Add some comments Don't allocate a root for the this-ref when binding methods if the this-ref is null

…gn C# and C's idea of marshal types better.

…m C#

ghost · 2022-02-04T11:01:00Z

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

kg added the arch-wasm WebAssembly architecture label Oct 22, 2021

kg requested review from pavelsavara and lewing October 22, 2021 14:33

pavelsavara reviewed Oct 26, 2021

View reviewed changes

src/mono/mono/sgen/sgen-memory-governor.c Show resolved Hide resolved

pavelsavara reviewed Oct 26, 2021

View reviewed changes

src/mono/wasm/runtime/invoke-api.ts Outdated Show resolved Hide resolved

pavelsavara reviewed Oct 26, 2021

View reviewed changes

src/mono/wasm/runtime/invoke-api.ts Outdated Show resolved Hide resolved

kg mentioned this pull request Oct 26, 2021

Extract various changes from the invoke and custom marshaler PRs #60877

Merged

kg force-pushed the bindings-new-invoke-api branch from 790fb64 to 1023050 Compare November 4, 2021 01:27

runfoapp bot mentioned this pull request Nov 4, 2021

Linker tests failing with no space left on device on Linux x64 #60927

Closed

kg force-pushed the bindings-new-invoke-api branch from 0011734 to 3c3555b Compare November 5, 2021 21:54

kg force-pushed the bindings-new-invoke-api branch from 3c3555b to 36f6556 Compare November 11, 2021 01:47

kg force-pushed the bindings-new-invoke-api branch from 6d40718 to dc95edb Compare November 20, 2021 12:10

kg added 8 commits December 8, 2021 14:34

Repair merge damage

def3119

Fix lint errors

aea0b6a

Don't generate dead code for void signatures

118e732

Checkpoint

bc5ab98

Remove some dead code and refactor bound method generator

aa08d51

Generate more specialized result handling code for bound methods. Ali…

96d5254

…gn C# and C's idea of marshal types better.

Codegen cleanup

18449e3

kg added 8 commits December 8, 2021 20:30

Satisfy analyzer

f470485

Fix GC safety issue

c7a8a26

Add explicit types in attempt to debug tsc performance issue

008a8f6

Fix tsc wasting 70 seconds on every build

b0e6586

Add basic build profiling information

b0ea873

Fix root index being incorrect

201cbf8

(squashed) prototype of new API for invoking JavaScript functions fro…

e7525f0

…m C#

Update imports

de3e4c1

kg force-pushed the bindings-new-invoke-api branch from dc95edb to de3e4c1 Compare December 11, 2021 04:13

jeffhandley added the area-System.Runtime.InteropServices.JavaScript label Jan 5, 2022

pavelsavara mentioned this pull request Jan 19, 2022

[wasm] implement roots on managed stack #63997

Merged

pavelsavara mentioned this pull request Jan 29, 2022

[draft][wasm] JSImport/JSExport prototype #64493

Closed

69 tasks

ghost closed this Feb 4, 2022

ghost locked as resolved and limited conversation to collaborators Mar 6, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUPER DRAFT] New API for invoking JS functions from C# #60765

[SUPER DRAFT] New API for invoking JS functions from C# #60765

kg commented Oct 22, 2021

dotnet-issue-labeler bot commented Oct 22, 2021

ghost commented Oct 22, 2021

kg commented Oct 22, 2021

kg commented Oct 24, 2021

pavelsavara Oct 26, 2021

kg Oct 26, 2021

pavelsavara Oct 26, 2021

kg Oct 26, 2021

pavelsavara commented Oct 26, 2021

kg commented Oct 26, 2021

kg commented Nov 5, 2021

kg commented Nov 5, 2021

ghost commented Feb 4, 2022

[SUPER DRAFT] New API for invoking JS functions from C# #60765

[SUPER DRAFT] New API for invoking JS functions from C# #60765

Conversation

kg commented Oct 22, 2021

dotnet-issue-labeler bot commented Oct 22, 2021

ghost commented Oct 22, 2021

kg commented Oct 22, 2021

kg commented Oct 24, 2021

pavelsavara Oct 26, 2021

Choose a reason for hiding this comment

kg Oct 26, 2021

Choose a reason for hiding this comment

pavelsavara Oct 26, 2021

Choose a reason for hiding this comment

kg Oct 26, 2021

Choose a reason for hiding this comment

pavelsavara commented Oct 26, 2021

kg commented Oct 26, 2021

kg commented Nov 5, 2021

kg commented Nov 5, 2021

ghost commented Feb 4, 2022