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

[browser] remove legacy js interop #96841

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 11, 2024

Public

  • MONO and BINDING JS APIs
  • disableDotnet6Compatibility JS flag
  • doc for HEAPXXX on emscripten module
  • dotnet-legacy.d.ts
  • MSBuild property WasmEnableLegacyJsInterop and related trimming support for it
  • semi-public mono_wasm_invoke_js_blazor() and WebAssembly.JSInterop.InternalCalls.InvokeJS() ICAL

Internal

  • any internal C# classes like Runtime and JSObject extensions DataView ...
  • perf tests related to legacy interop
  • unit tests System.Runtime.InteropServices.JavaScript.Legacy.Tests
    • moved timers.mjs, TimerTests
    • moved HttpRequestMessageTest
    • all modern scenarios are covered in System.Runtime.InteropServices.JavaScript.Tests
  • call_test_method used in some tests

TODO

Fixes #90044

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Jan 11, 2024
@pavelsavara pavelsavara added this to the 9.0.0 milestone Jan 11, 2024
@pavelsavara pavelsavara self-assigned this Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

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

Issue Details

Public

  • MONO and BINDING JS APIs
  • disableDotnet6Compatibility JS flag
  • doc for HEAPXXX on emscripten module
  • dotnet-legacy.d.ts
  • MSBuild property WasmEnableLegacyJsInterop and related trimming support for it
  • semi-public mono_wasm_invoke_js_blazor() and WebAssembly.JSInterop.InternalCalls.InvokeJS() ICAL

Internal

  • any internal C# classes like Runtime and JSObject extensions DataView ...
  • perf tests related to legacy interop
  • unit tests System.Runtime.InteropServices.JavaScript.Legacy.Tests
    • moved timers.mjs, TimerTests
    • moved HttpRequestMessageTest
    • all modern scenarios are covered in System.Runtime.InteropServices.JavaScript.Tests
  • call_test_method used in some tests

TODO

  • fix debugger tests which are still using bind_static_method in debugger-driver.html
Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 9.0.0

@pavelsavara

This comment was marked as outdated.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

@radical
In WBT I need Blazor version which contains dotnet/aspnetcore#52732, what do I need to do ?

@radical
Copy link
Member

radical commented Jan 15, 2024

You need an installer which references that version or newer of aspnetcore. We update the installer once a day via the darc flow. Just watch for that.

@pavelsavara
Copy link
Member Author

You need an installer which references that version or newer of aspnetcore. We update the installer once a day via the darc flow. Just watch for that.

The flow is aspnetcore -> sdk -> installer -> runtime, right ?

@pavelsavara
Copy link
Member Author

maybe this #96896

@pavelsavara pavelsavara force-pushed the browser_remove_legacy_js_interop branch from fd9796d to 9bc6437 Compare January 19, 2024 12:09
@pavelsavara pavelsavara marked this pull request as ready for review January 19, 2024 12:18
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

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.

Looks good to me 👍

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

I wonder, we left the legacy API in src/mono/wasm/Wasm.Build.Tests/data/test-main-7.0.js but the "dotnet-latest" we use for running wbt will not contain it anymore. @radical, do we ever run wbt with older dotnet?

@pavelsavara
Copy link
Member Author

I filled #97217 for

Assertion: should not be reached at D:\a\_work\1\s\src\mono\mono\mini\mini-llvm.c:1394

@pavelsavara pavelsavara merged commit 8c3ee30 into dotnet:main Jan 19, 2024
201 of 209 checks passed
@pavelsavara pavelsavara deleted the browser_remove_legacy_js_interop branch January 19, 2024 15:23
@radical
Copy link
Member

radical commented Jan 19, 2024

I wonder, we left the legacy API in src/mono/wasm/Wasm.Build.Tests/data/test-main-7.0.js but the "dotnet-latest" we use for running wbt will not contain it anymore. @radical, do we ever run wbt with older dotnet?

No. We can drop any 7.0 stuff.

@radekdoulik
Copy link
Member

it looks like this caused large blazor startup regression

image

the range is
aca77bd...5a6135e

@pavelsavara
Copy link
Member Author

The slowdown is strange, there is no new code in the startup sequence, just code removed.
Maybe something what was executed early is now executed later ?

There is also 80 KB reduction in uncompressed size, right ?

@radekdoulik
Copy link
Member

It might be something broken with the UI startup, the time is very close to 20 seconds. So I wonder if it is related to some timeout being set to 20s.

The reduced size is great and is visible in the cold startup.

@pavelsavara
Copy link
Member Author

I hope this will fix it #97305

Do we have same problem in the other benchmark suite too ?

@radekdoulik
Copy link
Member

It is indeed a timeout

Benchmark started<br/> [main.js:98:21](http://localhost:8844/main.js)
waitFor blazor: Rendered Index.razor timed out [main.js:192:25](http://localhost:8844/main.js)
MONO_WASM: Saved interp_pgo table to cache [logging.ts:22:12](file:///Users/rodo/git/runtime/src/mono/browser/runtime/logging.ts)
waitFor blazor: Rendered Index.razor timed out [main.js:192:25](http://localhost:8844/main.js)
AppStart, Blazor First UI count: 1, per call: 20307ms, total: 20.307s<br/>

tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser][MT] legacy JS interop needs GC boundaries on main thread too
5 participants