Skip to content

Wasm marshaler API for user-defined types; migrate Uri and DateTime to new API - #47640

Closed
kg wants to merge 3 commits into
dotnet:mainfrom
kg:bindings-new-marshaler-api
Closed

Wasm marshaler API for user-defined types; migrate Uri and DateTime to new API#47640
kg wants to merge 3 commits into
dotnet:mainfrom
kg:bindings-new-marshaler-api

Conversation

@kg

@kgkg commented Jan 29, 2021

Copy link
Copy Markdown
Contributor

This PR introduces a system for defining custom JS➔Managed and Managed➔JS marshaling primitives so that any* user-defined type can cross between environments seamlessly, and migrates our existing Uri and DateTime marshaling logic to use the new system.

First, some sample code:

publicstaticclassDateTimeMarshaler{publicstaticstringJavaScriptToInterchangeTransform=>"return value.toISOString()";publicstaticstringInterchangeToJavaScriptTransform=>"return new Date(value)";publicstaticDateTimeFromJavaScript(strings){returnDateTime.Parse(s).ToUniversalTime();}publicstaticstringToJavaScript(inDateTimedt){returndt.ToString("o");}}

General overview:

  • The author of the type exposes static methods with specific names and signatures on the type so that the wasm bindings layer can find them.
    • Right now there is a manual linker configuration to keep our built-in marshalers from being linked out. The user will need to do this themselves for any marshalers they define.
    • The runtime receives a table of type➔marshaler mappings at startup, and this table is generated by WasmAppBuilder.
    • The ideal form of this in the future will be a source generator that automatically finds marshalers and generates the linker configuration + table, but right now this is not possible (source generators can't do the things necessary for this yet).
  • The required methods are a JS➔Managed mapping and a Managed➔JS mapping. The types used by these methods have to (at present) be basic types that can cross through the bindings layer without assistance, i.e. double or string. In the future I hope to expand on this to allow marshaling spans or arrays.
  • In addition to the direct JS⬌Managed mapping, you can optionally define additional methods that return JS expression filters. Those expression filters will be evaluated on the JS side of the boundary to process the value - for example since neither JS Date or C# DateTime can cross the bindings boundary directly, you can marshal it as a string and then use a filter to map it to/from the JS Date type.
    • TODO: It would be ideal to expose these as const string instead of function getters, but right now the only straightforward way to get at managed values is through function calls.
  • Alongside all this we add a new 'a' type specifier for method signatures (the strings you pass to call_static_method, bind_static_method etc) that basically means 'figure it out'. When you use this specifier, the bindings layer will examine the target method and identify the best fit for that parameter. This is meant for the purpose of passing user-defined types so you don't need to think about whether they're classes or structs, but it also can be useful in other cases.
    • The downside is that any signature using this type specifier ends up being method-specific, so you will end up with method-specific generated code instead of the existing generated code that is shared by all methods with a given signature. There's room for some improvement here.

More detailed notes:

  • Before this PR we had zero support for passing structs across the JS⬌Managed boundary in any circumstances (other than DateTime).
  • The JS➔Managed conversion has to box the resulting value (if it's a struct), which introduces some unpleasant overhead. This ends up being less inefficient than you'd think (because currently we have to box all values being passed to managed methods from JS - not really sure why) but it's still an opportunity for performance improvement between eliminating some copies and ensuring the GC isn't involved.
  • The introduction of support for marshaling custom classes introduces a bit of overhead for classes without custom marshaling implementations, because of the additional check added to the existing Managed➔JS flow. However, the new check is extremely cheap and the existing flow was very slow 😊
  • There are corner cases where this new system will not run (throwing a runtime exception) or will otherwise fail (for example if the signature of your methods is weird you can get garbage values). I think many of these can be tightened up if we write more test cases and add more error handling and checks to match.
  • Many scenarios might call for an automated way to pass JSON blobs or raw byte data across the boundary. You can at least use filters (i.e. JSON.parse(value)) to simplify the former, but the latter really demands integrated support for passing Spans around and I have no idea how we'd do this in the existing setup.
  • We could consider having some sort of default fallback marshaling implementation based on BinaryFormatter or something like that, but it seems out of scope. This system is at least relatively easy to extend to do that as a fallback.

Also in this PR:

  • box_js_obj_with_converter API that allows you to request a specific managed type when boxing a JS value (this is how you create a type like Uri explicitly)
  • Caching for the existing 'find class in assembly' APIs, so that they can be used efficiently at runtime when trying to box values of a specific type
  • Caching for bind_method so that automated tests and benchmarks can safely bind methods at the start of each run without running out of runtime memory or incurring significant performance overhead (this was causing benchmarks to fail, because i had optimized the bindings so much that the run count in BDN was skyrocketing)
  • Additional test coverage for some of the existing code, like being able to pass JS Dates. I think that was actually broken, but Blazor works so whatever...
  • Miscellaneous performance improvements, producing around a ~4-6% performance gain on my benchmarks for existing call paths
  • Miscellaneous bug fixes and additional assertions in runtime/driver code (for things that previously would have produced crashes)
  • Better debug information for generated code so it's easier to examine in browser debuggers and profilers

@kgkg added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) arch-wasm WebAssembly architecture labels Jan 29, 2021
@ghost

Copy link
Copy Markdown

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

Issue Details

This PR introduces a system for defining custom JS➔Managed and Managed➔JS marshaling primitives so that any* user-defined type can cross between environments seamlessly. The ideal end goal is to use this new system for marshaling types like Uri, DateTime and Task so that they can be linked out if they are not used.

First, some sample code:

publicstructCustomDate{publicDateTimeDate;privatestaticstringJSToManaged_PreFilter()=>"value.toISOString()";privatestaticstringManagedToJS_PostFilter()=>"new Date(value)";privatestaticCustomDateJSToManaged(strings){returnnewCustomDate{Date=DateTime.Parse(s).ToUniversalTime()};}privatestaticstringManagedToJS(refCustomDatecd){returncd.Date.ToString("o");}}

General overview:

  • The author of the type exposes static methods with specific names and signatures on the type so that the wasm bindings layer can find them.
    • TODO: We need some straightforward way to ensure that the linker doesn't remove these methods from wasm builds unless the type itself is not referenced
  • The required methods are a JS➔Managed mapping and a Managed➔JS mapping. The types used by these methods have to (at present) be basic types that can cross through the bindings layer without assistance, i.e. double or string. In the future I hope to expand on this to allow marshaling spans or arrays.
  • In addition to the direct JS⬌Managed mapping, you can optionally define additional methods that return JS expression filters. Those expression filters will be evaluated on the JS side of the boundary to process the value - for example since neither JS Date or C# DateTime can cross the bindings boundary directly, you can marshal it as a string and then use a filter to map it to/from the JS Date type.
    • TODO: It would be ideal to expose these as const string instead of function getters, but right now the only straightforward way to get at managed values is through function calls.
  • Alongside all this we add a new 'a' type specifier for method signatures (the strings you pass to call_static_method, bind_static_method etc) that basically means 'figure it out'. When you use this specifier, the bindings layer will examine the target method and identify the best fit for that parameter. This is meant for the purpose of passing user-defined types so you don't need to think about whether they're classes or structs, but it also can be useful in other cases.
    • The downside is that any signature using this type specifier ends up being method-specific, so you will end up with method-specific generated code instead of the existing generated code that is shared by all methods with a given signature. There's room for some improvement here.

More detailed notes:

  • Before this PR we had zero support for passing structs across the JS⬌Managed boundary in any circumstances (other than DateTime).
  • The JS➔Managed conversion has to box the resulting value (if it's a struct), which introduces some unpleasant overhead. This ends up being less inefficient than you'd think (because currently we have to box all values being passed to managed methods from JS - not really sure why) but it's still an opportunity for performance improvement between eliminating some copies and ensuring the GC isn't involved.
  • The introduction of support for marshaling custom classes introduces a bit of overhead for classes without custom marshaling implementations, because of the additional check added to the existing Managed➔JS flow. However, the new check is extremely cheap and the existing flow was very slow 😊
  • There are corner cases where this new system will not run (throwing a runtime exception) or will otherwise fail (for example if the signature of your methods is weird you can get garbage values). I think many of these can be tightened up if we write more test cases and add more error handling and checks to match.
  • Many scenarios might call for an automated way to pass JSON blobs or raw byte data across the boundary. You can at least use filters (i.e. JSON.parse(value)) to simplify the former, but the latter really demands integrated support for passing Spans around and I have no idea how we'd do this in the existing setup.
  • We could consider having some sort of default fallback marshaling implementation based on BinaryFormatter or something like that, but it seems out of scope. This system is at least relatively easy to extend to do that as a fallback.

Also in this PR:

  • Additional test coverage for some of the existing code, like being able to pass JS Dates. I think that was actually broken, but Blazor works so whatever...
Author:kg
Assignees:-
Labels:

* NO MERGE *, arch-wasm

Milestone:-

@ghost

Copy link
Copy Markdown

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.

@kg

kg commented Jan 29, 2021

Copy link
Copy Markdown
ContributorAuthor

To help with understanding some of what this machinery does, here's an example of some of the code generated by the bindings to facilitate calling C# from JS:
https://gist.github.com/kg/f2c350b007d8e783085124c5d03f508e

@lewing

Copy link
Copy Markdown
Member

Failure is relevant

@radical

radical commented Feb 2, 2021

Copy link
Copy Markdown
Member

This breaks debugger tests with * Assertion: should not be reached at /Users/radical/dev/runtime/src/mono/mono/metadata/class-accessors.c:86.
To reproduce: $ make -C src/mono/wasm run-debugger-tests TEST_FILTER=DebuggerTests.ArrayTests.InvalidArrayId

It fails at this.async_method = Module.mono_bind_static_method ("[debugger-test] Math/NestedInMath:AsyncTest");
(https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/tests/debugger-test/debugger-driver.html#L14)

The method in question: public static async System.Threading.Tasks.Task<bool> AsyncTest(string s, int i)
(https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/tests/debugger-test/debugger-test.cs#L139)

.. is in a nested class Math.NestedInMath.

@kg

kg commented Feb 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks for digging in. I bet it's due to generics.

@kjpou1

Copy link
Copy Markdown
Contributor

Not sure if it is generics or not but I had to add some code for handling generic signatures in the PR here: #47519

This allows for generic signatures.

@kg
kgforce-pushed the bindings-new-marshaler-api branch from 2a4b472 to c8554ecCompareFebruary 5, 2021 12:19
@kjpou1

Copy link
Copy Markdown
Contributor

Just running through the DRAFT PR:

info: class_is_task
info: class_is_task
info: creating signature info for method result
info: creating signature info for method params
fail: [out of order message from the browser]: http://127.0.0.1:61482/dotnet.wasm 0:1703801 Uncaught RuntimeError: divide by zero
info: GC_MINOR: (Nursery full) time 2.40ms, stw 2.44ms promoted 259K major size: 1280K in use: 687K los size: 0K in use: 0K
info: GC_MINOR: (Nursery full) time 2.41ms, stw 2.43ms promoted 0K major size: 1280K in use: 687K los size: 0K in use: 0K
info: GC_MINOR: (Nursery full) time 3.01ms, stw 3.03ms promoted 0K major size: 1280K in use: 687K los size: 0K in use: 0K
info: GC_MINOR: (Nursery full) time 2.30ms, stw 2.32ms promoted 0K major size: 1280K in use: 687K los size: 0K in use: 0K

@kg
kgforce-pushed the bindings-new-marshaler-api branch 2 times, most recently from d4fef0c to 7d09a5dCompareFebruary 25, 2021 03:15
Base automatically changed from master to mainMarch 1, 2021 09:07
@kg
kgforce-pushed the bindings-new-marshaler-api branch 2 times, most recently from a439856 to 52d3615CompareMarch 19, 2021 04:10
@kg
kgforce-pushed the bindings-new-marshaler-api branch from 8897689 to 990a19fCompareMarch 23, 2021 20:16
Comment threadsrc/mono/wasm/debugger/DebuggerTestSuite/TestHarnessStartup.cs Outdated
@ghostghost closed this Apr 26, 2021
@ghost

Copy link
Copy Markdown

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@kgkg reopened this May 4, 2021
@kg
kgforce-pushed the bindings-new-marshaler-api branch from 4f05f98 to fd96dc3CompareMay 7, 2021 02:19
@kg
kgforce-pushed the bindings-new-marshaler-api branch from 5eca689 to 86b404bCompareMay 20, 2021 22:29
@lewing

Copy link
Copy Markdown
Member

@kg are you still missing some changes?

@kg

kg commented May 22, 2021

Copy link
Copy Markdown
ContributorAuthor

@kg are you still missing some changes?

I'm still working through the msbuild integration in a way that won't break tests

@kg
kgforce-pushed the bindings-new-marshaler-api branch from b69bcc0 to 7a1c854CompareNovember 10, 2021 16:54
Comment threadsrc/mono/wasm/runtime/method-binding.ts Outdated
@kg

kg commented Nov 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Here are a set of generated marshaler functions, @pavelsavara

//# sourceURL=https://mono-wasm.invalid/System_DateTime$FromJavaScript"use strict";constModule=__closure__.Module;constmono_wasm_new_root=__closure__.mono_wasm_new_root;const_create_temp_frame=__closure__._create_temp_frame;const_get_args_root_buffer_for_method_call=__closure__._get_args_root_buffer_for_method_call;const_get_buffer_for_method_call=__closure__._get_buffer_for_method_call;const_handle_exception_for_call=__closure__._handle_exception_for_call;const_teardown_after_call=__closure__._teardown_after_call;constmono_wasm_try_unbox_primitive_and_get_type=__closure__.mono_wasm_try_unbox_primitive_and_get_type;const_unbox_mono_obj_root_with_known_nonprimitive_type=__closure__._unbox_mono_obj_root_with_known_nonprimitive_type;constinvoke_method=__closure__.invoke_method;constmethod=__closure__.method;constthis_arg=__closure__.this_arg;consttoken=__closure__.token;constunbox_buffer=__closure__.unbox_buffer;constunbox_buffer_size=__closure__.unbox_buffer_size;constgetI32=__closure__.getI32;constgetU32=__closure__.getU32;constgetF32=__closure__.getF32;constgetF64=__closure__.getF64;constconverter_d_result_unmarshaled=__closure__.converter_d_result_unmarshaled;functionSystem_DateTime$FromJavaScript(arg0){_create_temp_frame();letresultRoot=token.scratchResultRoot,exceptionRoot=token.scratchExceptionRoot;token.scratchResultRoot=null;token.scratchExceptionRoot=null;if(resultRoot===null)resultRoot=mono_wasm_new_root();if(exceptionRoot===null)exceptionRoot=mono_wasm_new_root();letargsRootBuffer=_get_args_root_buffer_for_method_call(converter_d_result_unmarshaled,token);letscratchBuffer=_get_buffer_for_method_call(converter_d_result_unmarshaled,token);letbuffer=converter_d_result_unmarshaled.compiled_function(scratchBuffer,argsRootBuffer,method,arg0);letis_result_marshaled=false;resultRoot.value=invoke_method(method,this_arg,buffer,exceptionRoot.get_address());_handle_exception_for_call(converter_d_result_unmarshaled,token,buffer,resultRoot,exceptionRoot,argsRootBuffer);letresultPtr=resultRoot.value,result=undefined;result=resultPtr;_teardown_after_call(converter_d_result_unmarshaled,token,buffer,resultRoot,exceptionRoot,argsRootBuffer);returnresult;};returnSystem_DateTime$FromJavaScript;//# sourceURL=https://mono-wasm.invalid/interchange_filter_for_type62173044"use strict";constModule=__closure__.Module;constMONO=__closure__.MONO;constBINDING=__closure__.BINDING;consttypePtr=__closure__.typePtr;constalloca=__closure__.alloca;constgetI8=__closure__.getI8;constgetI16=__closure__.getI16;constgetI32=__closure__.getI32;constgetI64=__closure__.getI64;constgetU8=__closure__.getU8;constgetU16=__closure__.getU16;constgetU32=__closure__.getU32;constgetF32=__closure__.getF32;constgetF64=__closure__.getF64;constsetI8=__closure__.setI8;constsetI16=__closure__.setI16;constsetI32=__closure__.setI32;constsetI64=__closure__.setI64;constsetU8=__closure__.setU8;constsetU16=__closure__.setU16;constsetU32=__closure__.setU32;constsetF32=__closure__.setF32;constsetF64=__closure__.setF64;constSystem_DateTime$FromJavaScript=__closure__.System_DateTime$FromJavaScript;functioninterchange_filter_for_type62173044(value){switch(typeof(value)){case'number':
returnvalue;default:
if(valueinstanceofDate){returnvalue.valueOf();}elsethrownewError('Value must be a number (msecs since unix epoch), or a Date');}};returninterchange_filter_for_type62173044;//# sourceURL=https://mono-wasm.invalid/js_to_interchange_for_type62173044"use strict";constModule=__closure__.Module;constMONO=__closure__.MONO;constBINDING=__closure__.BINDING;consttypePtr=__closure__.typePtr;constalloca=__closure__.alloca;constgetI8=__closure__.getI8;constgetI16=__closure__.getI16;constgetI32=__closure__.getI32;constgetI64=__closure__.getI64;constgetU8=__closure__.getU8;constgetU16=__closure__.getU16;constgetU32=__closure__.getU32;constgetF32=__closure__.getF32;constgetF64=__closure__.getF64;constsetI8=__closure__.setI8;constsetI16=__closure__.setI16;constsetI32=__closure__.setI32;constsetI64=__closure__.setI64;constsetU8=__closure__.setU8;constsetU16=__closure__.setU16;constsetU32=__closure__.setU32;constsetF32=__closure__.setF32;constsetF64=__closure__.setF64;constSystem_DateTime$FromJavaScript=__closure__.System_DateTime$FromJavaScript;constfilter=__closure__.filter;functionjs_to_interchange_for_type62173044(value,method,parmIdx){letfilteredValue=filter(value);letconvertedResult=System_DateTime$FromJavaScript(filteredValue,method,parmIdx);returnconvertedResult;};returnjs_to_interchange_for_type62173044;

Comment threadsrc/mono/wasm/runtime/method-calls.ts Outdated
@kg

kg commented Nov 11, 2021 via email

Copy link
Copy Markdown
ContributorAuthor

@kg

kg commented Nov 20, 2021

Copy link
Copy Markdown
ContributorAuthor

PR updated with support for passing Span<byte> and ReadOnlySpan<byte> arguments to methods using the new 'b' signature character, and marshalers now explicitly request a scratch buffer instead of manually allocating it using alloca, which allows them to take Span arguments instead. Note that returning spans is not valid, since return values have to be boxed. I don't see this as a problem.

@azure-pipelines

Copy link
Copy Markdown
Pull request contains merge conflicts.

@kg
kgforce-pushed the bindings-new-marshaler-api branch 2 times, most recently from ceba251 to def3119CompareDecember 8, 2021 23:51
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kg

kg commented Dec 9, 2021

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-manual

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kg
kgforce-pushed the bindings-new-marshaler-api branch 3 times, most recently from 221f4c4 to b23df27CompareDecember 15, 2021 00:27
@kg

kg commented Dec 15, 2021

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-manual

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kg

kg commented Dec 15, 2021

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-manual

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kg
kgforce-pushed the bindings-new-marshaler-api branch 2 times, most recently from 8afdc4a to 0deb1c2CompareDecember 21, 2021 00:12
kg added 3 commits January 5, 2022 21:02
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
Don't generate dead code for void signatures
Remove some dead code and refactor bound method generator
Generate more specialized result handling code for bound methods. Align C# and C's idea of marshal types better.
Fix GC safety issue
Fix tsc wasting 70 seconds on every build
Add basic build profiling information
Fix root index being incorrect
Rename MemOffset and MemValue
Remove bind_method this_arg support
Move some stuff around
Change nested type descriptor syntax
…m hiding other errors, and avoid double teardown when managed code throws
@kg
kgforce-pushed the bindings-new-marshaler-api branch from fd1dc81 to 85eb349CompareJanuary 6, 2022 07:10
@kgkg closed this May 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Jun 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-Interop-monoNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@kg@lewing@radical@kjpou1@AndyAyersMS@marek-safar@pavelsavara@lambdageek@terrajobst