dotnet / runtimelab

This repo is for experimentation and exploring new ideas that may or may not make it into the main dotnet/runtime repo.
MIT License
1.38k stars 192 forks source link

[NativeAOT LLVM] Mark all `JSExport` wrapper methods as `UnmanagedCallersOnly` #2436

Closed maraf closed 8 months ago

maraf commented 9 months ago

Contributes to https://github.com/dotnet/runtimelab/issues/2434

Example of generated class ```c# // namespace System.Runtime.InteropServices.JavaScript { [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute] unsafe class __GeneratedInitializer { [global::System.ThreadStaticAttribute] static bool initialized; [global::System.Runtime.CompilerServices.ModuleInitializerAttribute] static internal void __Net7SelfInit_() { if (Environment.Version.Major == 7) { __Register_(); } } [global::System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute("__Wrapper_Greeting", "Xyz.Interop.MyClass", "BrowserConsoleApp")] static void __Register_() { if (initialized || global::System.Runtime.InteropServices.RuntimeInformation.OSArchitecture != global::System.Runtime.InteropServices.Architecture.Wasm) return; initialized = true; global::System.Runtime.InteropServices.JavaScript.JSFunctionBinding.BindManagedFunction("[BrowserConsoleApp]Xyz.Interop/MyClass:Greeting", 490867262, new global::System.Runtime.InteropServices.JavaScript.JSMarshalerType[] { global::System.Runtime.InteropServices.JavaScript.JSMarshalerType.Int32 }); } [global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(EntryPoint = "BrowserConsoleApp__GeneratedInitializer__Register_")] static void __Register__Export() { __Register_(); } } } namespace Xyz { public unsafe partial class Interop { public unsafe partial class MyClass { [global::System.Diagnostics.DebuggerNonUserCode] [global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(EntryPoint = "_5B_BrowserConsoleApp_5D_Xyz_Interop_2F_MyClass_3A_Greeting_490867262")] internal static unsafe void __Wrapper_Greeting(global::System.Runtime.InteropServices.JavaScript.JSMarshalerArgument* __arguments_buffer) { ref global::System.Runtime.InteropServices.JavaScript.JSMarshalerArgument __arg_exception = ref __arguments_buffer[0]; ref global::System.Runtime.InteropServices.JavaScript.JSMarshalerArgument __arg_return = ref __arguments_buffer[1]; int __retVal = default; try { __retVal = Xyz.Interop.MyClass.Greeting(); __arg_return.ToJS(__retVal); } catch (global::System.Exception ex) { __arg_exception.ToJS(ex); } // Marshal - Convert managed data to native data. __arg_return.ToJS(__retVal); } } } } ```
pavelsavara commented 9 months ago

Motivation is that NAOT can't use reflection to discover the wrapper, right ?

Could you please explain how would be the UCO symbol called ?

maraf commented 9 months ago

Could you please explain how would be the UCO symbol called ?

Example here https://github.com/maraf/MinimalDotNetWasmNativeAOT/blob/dotnetjs/DotnetJsHack/dotnet.runtime.js#L4939

pavelsavara commented 8 months ago

I have a question, for my education: why is the __Register_ method set up the way it is (i. e. why not call it from the generated wrapper under an initialization check)?

I'm not sure I understand the "under an initialization check", do you mean lazily ?

The reason is that JS side doesn't even know that the namespace and the function exist until they are bound by C# side.

Related: [ModuleInitializerAttribute] is anti-pattern in NAOT and should be avoided if at all possible.

Yes, we learned that lesson and fixed it for Net8 and later, where we only initialize it when JS calls const exports = await getAssemblyExports("assemblyName"); for particular assembly. But Net7 is not that smart and so we have this fallback.

The late bound approach avoids running many static constructors until you actually need it.

pavelsavara commented 8 months ago

@maraf looking at BrowserConsoleApp__GeneratedInitializer__Register UCO, I think we need to support assembly name with dots in it.

pavelsavara commented 8 months ago

Related: [ModuleInitializerAttribute] is anti-pattern in NAOT and should be avoided if at all possible.

Perhaps we could teach some tool to trim it for Net8 and above.

SingleAccretion commented 8 months ago

I'm not sure I understand the "under an initialization check", do you mean lazily ?

(Yes, void __Wrapper_Greeting() { if (!initialized) __Register_(); ... })

The reason is that JS side doesn't even know that the namespace and the function exist until they are bound by C# side.

And this is because getAssemblyExports doesn't ask for a particular export, but for all of them from a particular assembly, and you want to support "nice" JS syntax (assembly.ClassName.MyExport() instead of assembly.GetExport("ClassName", "MyExport").Invoke()-sort of thing), and you cannot "hook" JS object member resolution?

pavelsavara commented 8 months ago

you want to support "nice" JS syntax

Yes.

you cannot "hook" JS object member resolution?

Eventually we could use Proxy to do that hooking. It's known to be slow, but I'm not sure how much slow.

pavelsavara commented 8 months ago

We can gradually improve it. Problem is backward compatibility. To support code which people already generated with old version of the generator and it's inside of some nuget package. Or find the courage to break it for them.

SingleAccretion commented 8 months ago

Makes sense.

I don't personally see any big problem with this scheme. It does mean you have to eagerly initialize all the exports for a particular assembly, but perhaps there shouldn't be that many of them in the regular case.

pavelsavara commented 8 months ago

I wonder if runtimelab would self-consume this generator or keep using the runtime version (in SDK). Do you plan for merge this to runtime repo @maraf ?

SingleAccretion commented 8 months ago

I wonder if runtimelab would self-consume this generator or keep using the runtime version (in SDK).

There is also the question of how would you consume it outside runtime[lab]. Some sort of package reference override?

pavelsavara commented 8 months ago

I wonder if runtimelab would self-consume this generator or keep using the runtime version (in SDK).

There is also the question of how would you consume it outside runtime[lab]. Some sort of package reference override?

In Blazor we support non-AOT, non-workload dev loop (and also deployment). In that mode UCO would not show up at all as a native symbol in the .wasm file. So we need mono reflection there.

My concern is about codebase consistency.

jkotas commented 8 months ago

Perhaps we could teach some tool to trim it for Net8 and above.

System.Runtime.InteropServices.JavaScript ships "inbox" with netcoreapp, so the generator should be only concerned around with the current netcoreapp versions. It should not run for downlevel versions - the downlevel versions should use the generator that shipped with them. It means that the current (net9) version of the generator does not need to worry about targeting net7 or net8 runtime. Thoughts?

pavelsavara commented 8 months ago

Thoughts?

A) user code which used the generator in Net7 and the compiled code still lives in some nuget package. And should run with latest runtime.

B) new code generated with latest SDK to produce nuget package targeting Net7 need also work.

Are you saying that for B) the Net7 generator will be used and I don't need to worry about it anymore ?

pavelsavara commented 8 months ago

If so, this is worth generator patch for Net8, to not generate that ModuleInitializerAttribute.

We will need to find some alternative way how to keep this initializer code from always being trimmed. Because ILLink has no idea this will be called from JS. Currently ModuleInitializerAttribute keeps it alive.

jkotas commented 8 months ago

Are you saying that for B) the Net7 generator will be used and I don't need to worry about it anymore ?

Yes.

Try building Net7 app using .NET 8 SDK with /v:diag option. You should see JSImportGenerator or LibraryImportGenerator coming from Microsoft.NETCore.App.Ref 7.0.* .

jkotas commented 8 months ago

If so, this is worth generator patch for Net8, to not generate that ModuleInitializerAttribute.

We should do this for Net9. I am not sure whether it would meet the servicing bar for Net8.

pavelsavara commented 8 months ago

And it could be done here too. Because UCO will keep it alive, right ?

yowl commented 8 months ago

UCO will root the method as far as the Ilc method tree walking goes. I don't have any other concerns here, LGTM.

maraf commented 8 months ago

I wonder if runtimelab would self-consume this generator or keep using the runtime version (in SDK). Do you plan for merge this to runtime repo @maraf?

We should be able to override KnownFrameworkReference to get JSImportGenerator and S.R.IS.Javascript.dll (aka TargetingPack) built from runtimelab