Skip to content
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

[draft] JavaScript interop via [JSImport] and [JSExport] #41665

Closed
wants to merge 2 commits into from

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented May 12, 2022

Changes

  • converted all internal Blazor usage of interop to [JSImport] and [JSExport]
    • simplified interop for: Navigation, console logging, JSInProcessObjectReference dispose, WebAssemblyHostBuilder, RegisteredComponentsInterop
    • lazy assembly loading, satelite assembly loading: I changed it to deal with each file as separate interop call. Marshaling nested arrays of managed objects is not supported anymore.
    • DefaultWebAssemblyJSRuntime.InvokeDotNet - I think there could be follow up work which will make buffers transfer easier.
    • WebAssemblyJSRuntime.InvokeJS - I think there could be follow up work which will make buffers transfer easier. I don't fully understand the goal of BeginInvoke, EndInvoke. Note we support Task/Promise marshaling now.
    • WebAssemblyRenderer - this was GC hazzard as stays GC hazzard, the change is not making it more risky. It would be interesting to see if there is any perf impact. I hope it's faster.
    • WebAssemblyHotReload - I simplified dynamic import
  • GlobalExports.ts - I added few missing and few new methods. Those starting with upper case letter are [JSExport] bound managed methods, with JS native paramater types.
  • made IJSUnmarshalledObjectReference and IJSUnmarshalledRuntime obsolete
  • [JSImport] and [JSExport] are static method and they are not on class which could be mocked anymore. I commented 2 tests which relly on it. Leaving it for Blazor team to take next steps.
  • Added COOP headers to the sample app, so that subtle crypto could be tested
  • There is hotfix for the custom blazor runtime startup sequence, not big deal.

Testing

  • I manually tested hot reload, http
  • I don't have experience to do functional testing of affected Blazor components

Risks

  • performance
  • memory leaks: Luckily, none of the changed methods are marshaling object proxies.
  • we know that WS is slower for small messages. That's not related to this PR.

@pavelsavara pavelsavara added the feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly label May 12, 2022
@pavelsavara pavelsavara self-assigned this May 12, 2022
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label May 12, 2022
@mkArtakMSFT
Copy link
Member

@pavelsavara what is the state of this change? Is this ready for us to review already?

@pavelsavara
Copy link
Member Author

I will start working on it as soon as we land the preview7 SDK of the runtime in this repo.
It's on top of my list, stay tuned.

… `[JSExport]`

     - simplified interop for: Navigation, console logging, `JSInProcessObjectReference` dispose, `WebAssemblyHostBuilder`, `RegisteredComponentsInterop`
     - lazy assembly loading, satelite assembly loading: I changed it to deal with each file as separate interop call. Marshaling nested arrays of managed objects is not supported anymore.
     - `DefaultWebAssemblyJSRuntime.InvokeDotNet` - I think there could be follow up work which will make buffers transfer easier.
     - `WebAssemblyJSRuntime.InvokeJS` - I think there could be follow up work which will make buffers transfer easier. I don't fully understand the goal of `BeginInvoke`, `EndInvoke`. Note we support Task/Promise marshaling now.
     - `WebAssemblyRenderer` - this was GC hazzard as stays GC hazzard, the change is not making it more risky. It would be interesting to see if there is any perf impact. I hope it's faster.
     - `WebAssemblyHotReload` - I simplified dynamic import
 - `GlobalExports.ts` - I added few missing and few new methods. Those starting with upper case letter are `[JSExport]` bound managed methods, with JS native paramater types.
 - made `IJSUnmarshalledObjectReference` and `IJSUnmarshalledRuntime` obsolete
 - `[JSImport]` and `[JSExport]` are static method and they are not on class which could be mocked anymore. I commented 2 tests which relly on it. Leaving it for Blazor team to take next steps.
 - Added COOP headers to the sample app, so that subtle crypto could be tested
@pavelsavara pavelsavara marked this pull request as ready for review July 18, 2022 19:51
@pavelsavara pavelsavara requested a review from a team as a code owner July 18, 2022 19:51
@SteveSandersonMS
Copy link
Member

Regarding perf, I ran the Blazor WebAssembly perf benchmarks and found that this PR didn't really change the numbers in a conclusive way. That's great, since the worry was regression - this change isn't meant to improve perf for the existing code which largely relies on unmarshalled interop.

In a few of the test cases, this PR may be associated with about a 2% slowdown but it doesn't happen consistently and isn't far outside the margin of experimental error. Since the .NET-on-WebAssembly runtime itself has become generally faster in various ways in .NET 7 (both interpreted and AOT), I'm confident that people are still going to find the upgrade to .NET 7 to be a perf win.

@pavelsavara
Copy link
Member Author

Closing in favor of smaller #42814

@ghost
Copy link

ghost commented Jul 19, 2022

Hi @pavelsavara. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@pavelsavara pavelsavara added this to the .NET 8 Planning milestone Jul 20, 2022
@pavelsavara
Copy link
Member Author

I would love to re-open this for Net8 with your assistance on broken tests.

@ghost
Copy link

ghost commented Jul 20, 2022

Hi @pavelsavara. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@SteveSandersonMS
Copy link
Member

I would love to re-open this for Net8 with your assistance on broken tests.

Definitely. Even though we will need to leave the obsoleted unmarshalled interop APIs in place until after the next LTS release, we can stop using them in Blazor. Thanks for pushing this whole area forwards.

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 20, 2022

I realized that I could add bit of explanations:

  • the unmarshaler interface is implemented by icall in the runtime, there is no marshaling at all. The issue is that managed object would be retured as naked pointers. The old interop is almost necessary to deal with such raw pointers, including MonoString* etc. And only methods with _ref suffix are dealing with it correctly. Those are pretty difficult to use.
  • adopting the rest of this PR in blazor internals would protect you from GC/pointer related issues the old interop has. This was my stop-gap plan for multi-threading. I think this is a gap now, I have not realized yesterday.
  • when there is no dependency of runtime & blazor on the old interop marshallers (not the same as the unmarshalled icall), we could trim the size of the binaries. For those customers, which don't need the old interop themself and opt-in. Today it's not yet ready on runtime side either ...
  • I was happy with your performance conclusion in particular because we could now assume that RenderBatch call doesn't have to be that unmarshaled icall and could be [JSImport]. If it could be something else than IntPtr + direct memory access + GC pause, is separate issue now. With AOT maybe the binary serialization of it could be acceptable perf now ?

cc @SteveSandersonMS @lewing @lambdageek @danroth27

@ghost
Copy link

ghost commented Jul 20, 2022

Hi @pavelsavara. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants