Skip to content

[wasm] Lazy init of [JSExport] bindings #77293

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.

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

Merged
merged 10 commits into from
Dec 8, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Oct 20, 2022

The original code was generating the initializer into the same class as the exported method.
It triggers static constructors on classes with exported members.
That could be unexpected side-effect for the developers with performance or behavioral consequences.

  • The changed generator is creating static method __Register_
    • in single class per assembly constantly named System.Runtime.InteropServices.JavaScript.__GeneratedInitializer.
    • __Register_ method is now called when JS side requests bindings via getAssemblyExports API
  • We also generate __Net7SelfInit_ method with [ModuleInitializer].
    • It protects __Register_ from trimming.
    • It triggers __Register_ on Net7 runtime because it doesn't call __Register_ from JS.

This considers following combinations

  • code generated with Net7 generator running on Net8 runtime
    • it will not find __GeneratedInitializer class in the assembly and call module cctor instead.
  • code generated with Net8 generator running on Net7 runtime
    • call module cctor and that would call __Register_ on when on Net7
  • code generated with Net8 generator running on Net8 runtime
    • it will find __GeneratedInitializer class in the assembly and module cctor will be cheap empty operation
  • code generated with Net7 generator running on Net7 runtime
    • no change

Fixes #75598

@ghost
Copy link

ghost commented Oct 20, 2022

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

Issue Details

The original code was generating the initializer into the same class as the exported method.
Because the initializer is triggered by [ModuleInitializer] it also triggers static constructors on classes with exported members.

Next step could be to remove [ModuleInitializer] and call the method via mono_wasm_invoke_method_ref

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 8.0.0

@pavelsavara pavelsavara changed the title [wasm] Don't trigger static ctor with [JSExport] init [wasm] Lazy init of [JSExport] bindings Oct 21, 2022
@radekdoulik
Copy link
Member

The results look very good:

rodo@...:WasmPerformanceMeasurements/measurements/e596d3591e62b29c47ab19a11a07909ac60d9ed0$ find interp|grep results.json|xargs grep AppStart.*managed|grep -v AppBundle
interp/default/firefox/results.json:    "AppStart, Reach managed": 632.7
interp/default/chrome/results.json:    "AppStart, Reach managed": 484.2307692307692

These are close to times before regression:

interp/default/firefox    627ms
interp/default/chrome     438ms 

@pavelsavara

This comment was marked as resolved.

@pavelsavara
Copy link
Member Author

Generated code looks like this

// <auto-generated/>
namespace System.Runtime.InteropServices.JavaScript
{
    [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute]
    unsafe class __GeneratedInitializer
    {
        static bool initialized;
        [global::System.Runtime.CompilerServices.ModuleInitializerAttribute]
        static internal void __Net7SelfInit_()
        {
            if (Environment.Version.Major == 7)
            {
                __Register_();
            }
        }

        [global::System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute(
        "__Wrapper_ConsoleWriteLine_695648116", "System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper",
        "System.Runtime.InteropServices.JavaScript.Tests")]
        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(
"[System.Runtime.InteropServices.JavaScript.Tests]System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper:ConsoleWriteLine", 
            695648116, new global::System.Runtime.InteropServices.JavaScript.JSMarshalerType[]{
                        global::System.Runtime.InteropServices.JavaScript.JSMarshalerType.Discard, 
                        global::System.Runtime.InteropServices.JavaScript.JSMarshalerType.String});
        }
    }
}

namespace System.Runtime.InteropServices.JavaScript.Tests
{
    public unsafe partial class JavaScriptTestHelper
    {
        internal static unsafe void __Wrapper_ConsoleWriteLine_695648116(global::System.Runtime.InteropServices.JavaScript.JSMarshalerArgument* __arguments_buffer)
        {
            string message;
            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];
            // Setup - Perform required setup.
            ref global::System.Runtime.InteropServices.JavaScript.JSMarshalerArgument __message_native__js_arg = ref __arguments_buffer[2];
            // Unmarshal - Convert native data to managed data.
            __message_native__js_arg.ToManaged(out message);
            try
            {
                System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper.ConsoleWriteLine(message);
            }
            catch (global::System.Exception ex)
            {
                __arg_exception.ToJS(ex);
            }
        }
    }
}

@pavelsavara
Copy link
Member Author

@jkoritzinsky when net8 SDK is targeting Net7, does it use the analyzers from the net7 targeting pack or from net8 targeting pack ?

@vitek-karas
Copy link
Member

            global::System.Runtime.InteropServices.JavaScript.JSFunctionBinding.BindManagedFunction(
"[System.Runtime.InteropServices.JavaScript.Tests]System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTestHelper:ConsoleWriteLine", 
            695648116, new global::System.Runtime.InteropServices.JavaScript.JSMarshalerType[]{
                        global::System.Runtime.InteropServices.JavaScript.JSMarshalerType.Discard, 
                        global::System.Runtime.InteropServices.JavaScript.JSMarshalerType.String});

I assume the magical number is some kind of hash which we use to make sure to generate unique method name for the given method "wrapper".
What I don't understand is why we register the number - meaning that something somewhere in the runtime will have to use reflection to construct the method name and get it. Why not register the method itself as a delegate? It would avoid the name computation (the name would become an implementation detail of the source generator). It would also remove the need for DynamicDependency as there would be a direct code reference to the method.

@pavelsavara
Copy link
Member Author

The magical number is there just to differentiate multiple methods with same name but different signature.
Note that all __Wrapper_ConsoleWriteLine_* would have same C# signature. __Wrapper* is what we are calling from JS side.

Could we pass delegate to __Wrapper_ConsoleWriteLine_695648116 instead of name ?
I don't know for sure. I need something like MonoMethod* on JS side.
I also don't want the runtime to try to create any P/Invoke like wrappers around it, when creating native pointer from the C# delegate of the static method. From long time ago I remember that creating native function from delegate was expensive process which involves generating IL code for thunk and then JIT. I could be wrong.

It also has to work in AOT scenarios.

@pavelsavara
Copy link
Member Author

This is currently blocked on Net7/Net8 of <NetCoreAppCurrentVersion>7.0</NetCoreAppCurrentVersion>.
@lewing suggested that testing the compatibility would be difficult until it settles to 8.0.
Did I get it right Larry ?

@pavelsavara
Copy link
Member Author

pavelsavara commented Nov 9, 2022

To make this friendly to memory snapshot, we need to allow the JS side to enforce the binding refresh.
Also to make it work on multiple threads will need something similar.

Maybe static bool initialized need to be thread local (TLS).

# Conflicts:
#	src/mono/wasm/runtime/invoke-cs.ts
@pavelsavara
Copy link
Member Author

I tested forward and backward compatibility with Net7. I will leave fixing muti-threading for the next PR.

@pavelsavara pavelsavara marked this pull request as ready for review December 7, 2022 17:56
@pavelsavara pavelsavara requested a review from lewing as a code owner December 7, 2022 17:56
@pavelsavara pavelsavara requested a review from kg December 7, 2022 17:56
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

LGTM

@pavelsavara pavelsavara merged commit 05bf5c3 into dotnet:main Dec 8, 2022
@pavelsavara
Copy link
Member Author

image

@ghost ghost locked as resolved and limited conversation to collaborators Jan 11, 2023
@pavelsavara pavelsavara deleted the wasm_jsexport_fix branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] app startup regression
6 participants