Use mono_bind_static_method for invoking JS methods #17942

pranavkm · 2019-12-17T23:02:54Z

No description provided.

SteveSandersonMS · 2019-12-18T14:35:16Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

-      Module.stackRestore(stack);
-    }
+    const invokeEntrypoint = bindStaticMethod('Microsoft.AspNetCore.Blazor', 'Microsoft.AspNetCore.Blazor.Hosting.EntrypointInvoker', 'InvokeEntrypoint');
+    invokeEntrypoint(assemblyName, null);


Is it really right to pass null here as the second param? Shouldn't we be passing an empty string[], e.g., the result of calling mono_string_array_new(0)?

AFAIK, mono_bind_static_method converts JS values to the .NET ones. So we'd have to pass in an empty JS array. Let me play around with this.

Running in to this issue: mono/mono#18245 if I try passing in an array. I'm passing in a null now and null-coalescing it in InvokeEntrypoint. I think we can work out the WASM folks to figure out a nice way to pass arbitrary types here.

SteveSandersonMS · 2019-12-18T14:39:42Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

 let mono_string_get_utf8: (managedString: System_String) => Mono.Utf8Ptr;
-let mono_string: (jsString: string) => System_String;
+let mono_bind_static_method: (fqn: string) => (args: (System_Object | null)[]) => System_Object;


I don't think it's necessary to declare this as a separate variable, since it already exists as Module.mono_bind_static_method.

Instead, consider editing MonoType.d.ts (in the same dir as this file) and add an entry inside the definition of Module to match this function. Then it will be possible to call Module.mono_bind_static_method directly and in a strongly-typed way anywhere else.

Oh wait I see you already did add that into MonoTypes.d.ts. So you could just use that directly and don't have to create another variable referencing it here.

Did you do this for perf? If so I suspect it's not worth it. We normally cache the result of calling mono_bind_static_method so we don't need an extra layer of caching for just the evaluation of Module.mono_bind_static_method.

SteveSandersonMS · 2019-12-18T14:47:41Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

@@ -346,10 +244,16 @@ function getArrayDataPointer<T>(array: System_Array<T>): number {
  return <number><any>array + 12; // First byte from here is length, then following bytes are entries
 }

+function bindStaticMethod(assembly: string, typeName: string, method: string) : (...args: any[]) => any {


Shouldn't this be typed as returning (...args: (System_Object | null)[]) => System_Object and not (...args: any[]) => any?

I suspect it would be good to declare a type referencing a bound method. For example, in MonoType.d.ts, add something like:

type BoundStaticMethod = (...args: (System_Object | number | null)[]) => (System_Object | number | null);

... and then mono_bind_static_method can be declared as:

function mono_bind_static_method(fqn: string): BoundStaticMethod;

... and then any other uses of it can reference BoundStaticMethod as appropriate too.

From what I can tell, mono_bind_static_method does the JS to .NET type conversion on our behalf: https://github.com/mono/mono/blob/master/sdks/wasm/src/binding_support.js#L649-L657 and the reverse with the result. You're passing in \ reading JS types, not .NET types.

That said, we could limit it to the subset of types we expect to pass around which is strings or numbers.

SteveSandersonMS · 2019-12-18T14:50:12Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

+        methodIdentifier,
+        dotNetObjectId ? dotNetObjectId.toString() : null,
+        argsJson,
+      ) as string;


This looks a lot better!

SteveSandersonMS · 2019-12-18T14:58:34Z

src/Components/Web.JS/src/Platform/Platform.ts

@@ -26,7 +22,6 @@ export interface Platform {
 // We don't actually instantiate any of these at runtime. For perf it's preferable to
 // use the original 'number' instances without any boxing. The definitions are just
 // for compile-time checking, since TypeScript doesn't support nominal types.
-export interface MethodHandle { MethodHandle__DO_NOT_IMPLEMENT: any }
 export interface System_Object { System_Object__DO_NOT_IMPLEMENT: any }
 export interface System_String extends System_Object { System_String__DO_NOT_IMPLEMENT: any }
 export interface System_Array<T> extends System_Object { System_Array__DO_NOT_IMPLEMENT: any }


It's interesting that you're able to remove things from here. It looks to me now that the Platform interface isn't actually used as a concept. It originally existed in the very first days of Blazor when moving from .NET Anywhere to Mono. We had an abstraction over different .NET WebAssembly runtimes, but I think that no longer serves any purpose.

Please feel free to ignore this fact if you want - it doesn't have to be part of this PR or dealt with any time soon really - but if you're in the mood for cleaning up this code then feel free to delete Platform.ts and Environment.ts altogether. The two bits of code that need to access the platform instance could directly refer to the singleton monoPlatform exported by Platform/Mono/MonoPlatform instead.

Update: on further consideration, it is quite nice to keep the Platform interface so that we don't unnecessarily couple other bits of implementation to Mono-specific concepts. For example, in SharedMemoryRenderBatch, we might have been tempted to hard-code Mono knowledge like the number of bytes offset into a System_Array at which the first entry can be found. Having the Platform abstraction makes it clearer that Mono concepts should only live in one central place.

Given that there's no runtime cost to having an extra TypeScript interface, I'd lean towards leaving this as-is.

I'll leave MethodHandle in for completeness.

SteveSandersonMS

I've added some minor suggestions, but this looks great!

Were you able to verify via the benchmarks app that this doesn't affect perf? I don't expect it would, but I guess it's technically possible that Mono's alternative parameter-passing logic could be a lot more expensive due to doing extra checks or something.

As long as the perf doesn't seem noticably difference according to the rendering benchmark, I think this is a great improvement.

pranavkm · 2019-12-18T20:18:11Z

As long as the perf doesn't seem noticably difference according to the rendering benchmark, I think this is a great improvement.

Yup, no changes in the benchmark results.

chucker · 2020-02-24T12:30:22Z

This PR seems to have removed Blazor.platform.findMethod (and two others) from Platform.ts. Is there a supported replacement? Is there a TypeScript npm package I can reference to get the current version of the interfaces?

SteveSandersonMS · 2020-02-24T13:42:48Z

Mono WebAssembly now exposes APIs under BINDING.

Example of bind-then-call:

var method = BINDING.bind_static_method('[mscorlib] System.IO.Path:Combine');
method('abc', 'def');

Example of doing it in one step:

BINDING.call_static_method('[mscorlib] System.IO.Path:Combine', ['abc', 'def']);

There's a lot of fine-grained control and capabilities now exposed as APIs on BINDING.*. Have a look in your browser's autocomplete list for details:

If you want to know which parts of this are documented or supported for public use, please check at the https://github.com/mono/mono repo.

As for Blazor's JS APIs, there isn't a supported .d.ts file but we do have it on the backlog at #5508.

Use mono_bind_static_method for invoking JS methods

670886d

pranavkm requested a review from SteveSandersonMS as a code owner December 17, 2019 23:02

SteveSandersonMS reviewed Dec 18, 2019

View reviewed changes

SteveSandersonMS approved these changes Dec 18, 2019

View reviewed changes

Pilchie added the area-blazor Includes: Blazor, Razor Components label Dec 18, 2019

Changes per PR

a2515ed

pranavkm merged commit 62e2fb2 into blazor-wasm Dec 18, 2019

pranavkm deleted the prkrishn/do-more-mono branch December 18, 2019 20:18

chucker mentioned this pull request Feb 24, 2020

3.2 broke it again :( Lupusa87/BlazorWebSocketHelper#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use mono_bind_static_method for invoking JS methods #17942

Use mono_bind_static_method for invoking JS methods #17942

Uh oh!

pranavkm commented Dec 17, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019 •

edited

Loading

Uh oh!

pranavkm Dec 18, 2019

Uh oh!

pranavkm Dec 18, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019

Uh oh!

pranavkm Dec 18, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019 •

edited

Loading

Uh oh!

SteveSandersonMS Dec 18, 2019 •

edited

Loading

Uh oh!

pranavkm Dec 18, 2019

Uh oh!

SteveSandersonMS left a comment

Uh oh!

pranavkm commented Dec 18, 2019

Uh oh!

chucker commented Feb 24, 2020

Uh oh!

SteveSandersonMS commented Feb 24, 2020 •

edited

Loading

Uh oh!

Uh oh!

Use mono_bind_static_method for invoking JS methods #17942

Use mono_bind_static_method for invoking JS methods #17942

Uh oh!

Conversation

pranavkm commented Dec 17, 2019

Uh oh!

SteveSandersonMS Dec 18, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Dec 18, 2019 • 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!

SteveSandersonMS Dec 18, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Dec 18, 2019

Uh oh!

chucker commented Feb 24, 2020

Uh oh!

SteveSandersonMS commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SteveSandersonMS Dec 18, 2019 •

edited

Loading

SteveSandersonMS Dec 18, 2019 •

edited

Loading

SteveSandersonMS Dec 18, 2019 •

edited

Loading

SteveSandersonMS commented Feb 24, 2020 •

edited

Loading