Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
pavelsavara wants to merge 2 commits into from
Closed

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

pavelsavara wants to merge 2 commits into from

Conversation

@pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 29, 2022 •
edited
Loading

Design

This steals many ideas from #60765 and also from conversations with @kg

  • only C# and JS, no C code.
    • use stackalloc
    • C# generated on compile time, JS generated on runtime
  • Never pass MonoObject* to user javaScript
    • use GCHandle for C# objects
    • use JSHandle for JS objects
    • internally MonoString* is still used for strings and exception Message
  • use of typeof(T).TypeHandle as method signature
    • structure of the buffers is opaque to the public API
    • signature buffer: [TRes,T1,T2,T3, ...]
      • type handle, extra buffer length and offset, use of GC root
    • data buffer: [res,t1,t2,t3, ...]
      • discriminated union: actual type, primitive values, GCHandle/JSHandle, GCRoot, extra buffer ptr
  • there are registered marshallers at both sides of the C#/JS boundary
    • JS boundary is not crossed to marshal individual argument
    • primitive types are not boxed
    • generated C# code inline the correct marshaller method (without the hash table lookup or interface call)
  • could be also executed without generator, with caveats
    • dynamic call has to lookup the marshaller by TypeHandle in hash table
    • dynamic call to argument marshaller has to box/cast the argument object

Out of scope

  • generic methods
  • instance (non-static) methods
  • in, out, ref, [In], [Out], [Ref], [MarshalAs]

TODO

  • code generator
    • dummy JavaScript.MarshalerGenerator
    • write code by hands JSImport
    • write code by hands JSExport
    • actual generator JSImport
    • actual generator JSExport
    • add localized gererator messages for unsupported scenarios
  • custom marshalling
    • install custom marshaller only for current method
    • install custom JS code
    • sample ExtraBuffer with Vector3
  • Generator
    • Consider MarshalUsing attribute
    • It could be per parameter!
    • What is the error experience for things that are out-of-scope? And if they will become in-scope, what is the light-up experience?
    • What analyzer/fixers might be added to help users do the ‘right’ thing when using the new APIs?
  • API
    • extension methods for interface IJSObject, set property, get property
    • custom marshaller on JSExport/JSImport attributes
    • C# API to create new JS function from string
    • unify naming, upercase, args numbering
    • move to System.Runtime.InteropServices.JavaScript - drop Private
    • add ref assembly
    • pass API review with anything currently in System\Runtime\InteropServices\JavaScript\Public\**
    • in API proposal PR mention how internal message structure was designed and why. why not protobuf.
  • all necessary type marshallers, both directions
    • Nullable types
    • Int32, Int64, Double <=> Number
    • System.String <=> string
    • System.Object, JSObject <=> ManagerObject, object
    • System.Exception,JSException <=> Error, ManagedError
    • DateTime <=> Date
    • DateTimeOffset <=> Date
    • Task, ValueTask <=> Promise
    • IntPtr <=> VoidPtr
    • void*, byte* <=> ??? by ref or by value ?
    • Span<byte> <=> ???
    • byte[] <=> ???
    • URI <=> string as custom marshaller only
    • Delegate <=> function do we really need it ? It opens us to dispatch to dynamic C# signatures on runtime.
    • Decimal (16 bytes) <==> Number ?
  • co-exist with current System.Private.Runtime.InteropServices.JavaScript implementation
    • drop old proxies like TypedArray, Map and HostObject
    • drop InFlight
  • Low level
    • replace cwrap+ccall+mono_runtime_invoke in BindCSFunctionByName with something faster
    • jsHandle to function - wrap with JSObject so that GC works. Same for method GCHandle.
    • dealocate JS roots after call to C#
    • debug only assert, via terser pipeline
    • replace mono_wasm_new_root_buffer with pool+stack based allocator
  • replace old code path with new
    • BINDING.bind_static_method -> JSExport.
      • Only to C# methods marked with JSExport, otherwise we are open to dynamic C# signatures on runtime.
    • update existing unit tests to use new code path
    • icalls in BrowserWebSocket implementation
    • blazor calls
    • wrapped_cs_functions
  • asserts/tests
    • unit test JSImport
    • unit test JSExport
    • make sure that it can AOT well!
    • assert expected type of instance on JS side
    • measure number of ccalls
    • measure interp/AOT transitions
    • provide more detailed message about what failed why (some parameters passed from JS may fail to cast)
    • fill dead roots with garbage, to make sure something fails
    • implement micro benchmark in Wasm.BrowserProfile.Sample
    • measure perf
    • test what happens with JS view after WASM resize. Is it a copy ?

@pavelsavara pavelsavara added NO-REVIEW Experimental/testing PR, do NOT review it arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels Jan 29, 2022
@pavelsavara pavelsavara added this to the 7.0.0 milestone Jan 29, 2022
@pavelsavara pavelsavara requested a review from kg January 29, 2022 17:28
@ghost
Copy link

ghost commented Jan 29, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned pavelsavara Jan 29, 2022
@ghost
Copy link

ghost commented Jan 29, 2022

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

Issue Details

This steals many ideas from #60765 and also from conversation with @kg

Design

  • only C# and JS, no C code.
    • use stackalloc
    • Both sides could be eventually generated.
  • use only array of typeof(T).TypeHandle.Value as method signature
    • signature buffer: [TRes,T1,T2,T3]
    • data buffer: [res*,t1*,t2*,t3*]
  • Never pass MonoObject* to javaScript
    • use GCHandle for C# objects
    • use JSHandle for JS objects
    • MonoString* is used for exceptions
    • MonoString* is still used for strings
    • this may eliminate
  • there are registered marshallers at both sides of the C#/JS boundary
    • boundary is not crossed to marshal individual argument
    • dynamic call has to lookup the marshaller by TypeHandle in hash table
    • dynamic call to marshaller has to box/cast the object
    • primitive types are not boxed
    • generated code could eventually inline the marshaller method, without the lookup

Open questions

  • what would be good alternative to MonoString* so that we don't have to GC root any args at all ?
  • shall we put IJSObject to public API?
  • The return of the generic method InvokeJSFunctionByName could return null, but it doesn't

TODO

  • add JSException to public API
  • all remaining marshallers, this only drafted few
  • design of JS->C# call and binding using the same pattern
  • co-exist with current System.Private.Runtime.InteropServices.JavaScript implementation ?
  • type buffers could be cached in dynamic invoke and generated could could have them as static field.
Author: pavelsavara
Assignees: -
Labels:

NO REVIEW, arch-wasm, area-System.Runtime.InteropServices.JavaScript
Milestone: 7.0.0

@kg

This comment was marked as resolved.

Sign in to view
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
kg
kg reviewed Feb 1, 2022
View reviewed changes
@pavelsavara

This comment was marked as resolved.

Sign in to view
@kg

This comment was marked as resolved.

Sign in to view
@pavelsavara

This comment was marked as resolved.

Sign in to view
@runfoapp runfoapp bot mentioned this pull request Feb 3, 2022
Closed
@ViktorHofer ViktorHofer mentioned this pull request Feb 8, 2022
Merged
@pavelsavara pavelsavara changed the title [draft][wasm] InvokeJSFunctionByName alternate design [draft][wasm] JSImport/JSExport prototype Feb 9, 2022
marek-safar
marek-safar reviewed Feb 18, 2022
View reviewed changes
src/libraries/System.Private.Runtime.InteropServices.JavaScript/gen/Directory.Build.props Outdated
Copy link
Contributor

@marek-safar marek-safar Feb 18, 2022

Choose a reason for hiding this comment

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

I'd put .Private to the end so you can use same prefix for namespace aswell

This was referenced Mar 2, 2022
Closed
Closed
Closed
@pavelsavara pavelsavara force-pushed the wasm_new_invoke_js3 branch from 4f6886c to aacf17f Compare March 31, 2022 17:24
@pavelsavara
Merge commit 'dbc966aa2dca791a145a08c227b06ba23886e876' into wasm_new…
35a3a05 
…_invoke_js3

# Conflicts:
#	src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs
#	src/mono/wasm/runtime/corebindings.ts
#	src/mono/wasm/runtime/cwraps.ts
#	src/mono/wasm/runtime/exports.ts
#	src/mono/wasm/runtime/method-binding.ts
#	src/mono/wasm/runtime/startup.ts
#	src/mono/wasm/runtime/types.ts
This was referenced Apr 7, 2022
Closed
Closed
@ghost ghost added the no-recent-activity label Apr 21, 2022
@ghost
Copy link

ghost commented Apr 21, 2022

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented May 6, 2022

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this May 6, 2022
@pavelsavara pavelsavara mentioned this pull request May 30, 2022
Merged
74 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Jun 5, 2022
@pavelsavara pavelsavara deleted the wasm_new_invoke_js3 branch July 14, 2022 20:45
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kg kg kg left review comments

@marek-safar marek-safar marek-safar left review comments

@jkotas jkotas jkotas left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@lewing lewing Awaiting requested review from lewing lewing will be requested when the pull request is marked ready for review lewing is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@pavelsavara pavelsavara

Labels

arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript new-api-needs-documentation no-recent-activity NO-REVIEW Experimental/testing PR, do NOT review it

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@pavelsavara @kg @marek-safar @jkotas @ViktorHofer