Skip to content

Conversation

@pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 23, 2022

Implements interpreted new JS interop

instead of generating JS code and new Function which is not CSP compliant

Fixes #68374

  • test performance
  • test with policy enabled

(this doesn't fix eval in legacy interop, which is used in Blazor in Net7)

@pavelsavara pavelsavara added this to the 8.0.0 milestone Aug 23, 2022
@pavelsavara pavelsavara requested review from maraf and radical August 23, 2022 16:43
@pavelsavara pavelsavara self-assigned this Aug 23, 2022
@ghost
Copy link

ghost commented Aug 23, 2022

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

Issue Details

Implements interpreted JS interop

instead of generating JS code and new Function which is not CSP compliant

Fixes #68374

TODO

  • test performance
  • test with policy enabled
Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 8.0.0

@pavelsavara pavelsavara requested a review from kg August 23, 2022 16:47
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Member

kg commented Aug 23, 2022

Did you evaluate the alternative of pre-generating all the JS at build time and shipping a dictionary of it? The interpreter performance is a mild concern for me.

@pavelsavara
Copy link
Member Author

Did you evaluate the alternative of pre-generating all the JS at build time and shipping a dictionary of it? The interpreter performance is a mild concern for me.

I want to measure the difference of this approach first, to see if it's worth it.

  • The complexity deployment of pre-generated files is nightmare compared to this.
  • considering deployment from NuGet packages
  • exposing more JS API surface (with backward compatibility)
  • also it would increase download size, probably minor concern

We could also do it on link-time, when wasm-tools are installed on dev machine. But this is the simplest thing to try.

@pavelsavara

This comment was marked as outdated.

@pavelsavara

This comment was marked as outdated.

@pavelsavara
Copy link
Member Author

For context: Blazor in Net7 is using legacy interop, which is not CSP safe. It's too late/risky to do anything about it now.

We have following options:

For the new interop in Net8
A) make CSP-safe default, keep generator in as opt-in for extra perf.
B) keep generator default, add CSP-safe as opt-in.
C) keep only CSP-safe version. This is my preference long term.

For new interop in Net7, we could back-port this PR or not.
It depends if there are users which would benefit from it (outside Blazor)

For legacy interop, we could eventually implement the same trick. But I would rather not invest into it and get rid of it soon. We will get rid of it from Blazor codebase in Net8 I hope.

@pavelsavara
Copy link
Member Author

pavelsavara commented Aug 24, 2022

Chrome on Windows, Release+AOT

measurement generated interpreted interpreted JSImport optimized
JSInterop, JSExportInt 4.0423ms 4.2627ms 4.5489ms
JSInterop, JSExportString 23.5286ms 23.0558ms 23.3632ms
JSInterop, JSImportInt 0.5596ms 0.8967ms 0.4345ms
JSInterop, JSImportString 28.2426ms 26.8551ms 26.0259ms
JSInterop, JSImportTask 94.2812ms 93.7385ms 90.8788ms
JSInterop, JSImportTaskFail 510.8421ms 497.1000ms 502.9500ms
JSInterop, JSImportFail 165.4750ms 166.3659ms 170.3590ms

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

image

@pavelsavara pavelsavara marked this pull request as ready for review August 30, 2022 13:55
@pavelsavara pavelsavara requested a review from lewing as a code owner August 30, 2022 13:55
@pavelsavara pavelsavara requested a review from kg August 30, 2022 13:55
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Member

kg commented Aug 31, 2022

Please do a basic test in a couple browsers (Chrome + Safari or Chrome + Firefox would be fine) running interop invokes in a loop with this new implementation to make sure it doesn't end up causing frequent GCs, since we know that the type info for the bound functions will now be bad (vs before where each generated wrapper was statically typed). As long as it doesn't produce a bunch of allocations, I'm OK with removing the codegen since you did some perf benchmarks and the numbers looked OK

@pavelsavara pavelsavara requested a review from kg August 31, 2022 20:07
@pavelsavara
Copy link
Member Author

This is Chrome's heap during the stress test
image

@pavelsavara
Copy link
Member Author

And this is FireFox diff of memory snapshots on the same benchmark

image

@pavelsavara pavelsavara merged commit a919d61 into dotnet:main Sep 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2022
@pavelsavara pavelsavara deleted the wasm_csp branch October 13, 2022 10:03
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.

Make mono CSP Compliant

3 participants