[browser] fix trimming of `LegacyExports` with `WasmEnableLegacyJsInterop` by pavelsavara · Pull Request #83664 · dotnet/runtime

pavelsavara · 2023-03-20T10:15:34Z

This is 2nd attempt on enabling trimming of legacy interop, in which

there is visibility chain from JSFunctionBinding.BindJSFunction -> LegacyExportsTrimmingRoot -> LegacyExports by default.
there is ILLInk substitution file ILLink.Substitutions.LegacyJsInterop.xml which is cutting the chain at LegacyExportsTrimmingRoot.TrimWhenNotWasmEnableLegacyJsInterop
- it is used only when there is wasm workload and the WasmEnableLegacyJsInterop is set to false

Other changes:

test in advanced sample
fix of #if DISABLE_LEGACY_JS_INTEROP in native code
new variable nativeLegacyJsInterop which could be set during link time with workload on customer machine
test in System.Runtime.InteropServices.JavaScript.Tests

ghost · 2023-03-20T10:15:51Z

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

Issue Details

There is problem with E2E test of blazor.
The reason is that the original implementation doesn't have logic nor the file ILLink.Descriptors.LegacyJsInterop.xml available when the workload is not available, when compiling Blazor.

This is alternate implementation, in which

there is visibility chain from JSFunctionBinding.BindJSFunction -> LegacyExportsTrimmingRoot.PreventTrimming -> LegacyExports.
there is ILLInk substitution which is cutting the chain at LegacyExportsTrimmingRoot.PreventTrimming, only when there is workload and the WasmEnableLegacyJsInterop is set to false

Author:	pavelsavara
Assignees:	pavelsavara
Labels:	`arch-wasm`, `area-System.Runtime.InteropServices.JavaScript`
Milestone:	8.0.0

maraf

My questions above are non-blocking

pavelsavara · 2023-03-20T13:09:25Z

@vitek-karas is there nicer way how to create visibility chain (to multiple public methods) which could be easily trimmed with one substitution file ?
Please see LegacyExports.cs and ILLink.Substitutions.LegacyJsInterop.xml on this PR. Thank you.

pavelsavara · 2023-03-20T14:02:59Z

/azp run runtime-wasm

azure-pipelines · 2023-03-20T14:03:11Z

Azure Pipelines successfully started running 1 pipeline(s).

amcasey · 2023-03-20T22:48:37Z

Who's driving this now that @pavelsavara is (I understand) out?

kg · 2023-03-20T22:52:27Z

Who's driving this now that @pavelsavara is (I understand) out?

I don't know if a hand-off was arranged or not. I can kick off an email chain if necessary.

amcasey · 2023-03-20T22:58:14Z

Who's driving this now that @pavelsavara is (I understand) out?
I don't know if a hand-off was arranged or not. I can kick off an email chain if necessary.

Could you? aspnetcore is a little nervous about shipping without having taken an update in two weeks. Thanks!

pavelsavara · 2023-03-20T23:08:05Z

@maraf will take it from here tomorrow morning.
I could not figure out last failure on #83664 and I'm off in 2 hours.

The minimal #83637 is also acceptable way to unblock this.

pavelsavara · 2023-03-20T23:16:30Z

@maraf

Wasm.Console.Bench.Sample and Wasm.BrowserProfile.Sample are failing in wasm-ld to link mono_wasm_invoke_js_with_args_ref and other legacy symbols. I have no idea why

maraf · 2023-03-21T17:09:49Z

We can't figure it out. It seems that emcc has some side effects on builds running in parallel. The order in which samples are compiled matters. Locally it runs even when compiling samples in parallel.

maraf · 2023-03-21T20:11:45Z

note: In the last run https://dev.azure.com/dnceng-public/public/_build/results?buildId=212475&view=results the advanced sample ran after bench and profile samples

vitek-karas · 2023-03-24T20:43:44Z

@vitek-karas is there nicer way how to create visibility chain (to multiple public methods) which could be easily trimmed with one substitution file ?

I think this is fine, given the requirements. It's not nice, but I can't think of a much better way. The ask is a bit weird though :-).

# Conflicts: # src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in

pavelsavara · 2023-04-18T12:03:52Z

/azp run runtime-wasm

azure-pipelines · 2023-04-18T12:04:08Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-04-18T14:33:11Z

CI failures are unrelated

maraf

Looks good to me 👍

pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels Mar 20, 2023

pavelsavara added this to the 8.0.0 milestone Mar 20, 2023

pavelsavara requested a review from maraf March 20, 2023 10:15

pavelsavara self-assigned this Mar 20, 2023

maraf reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/mono/wasm/build/WasmApp.targets Outdated

This was referenced Mar 20, 2023
[main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/aspnetcore#47110
Merged
[browser] partial reversal of PR #82826 #83637
Merged

pavelsavara marked this pull request as ready for review March 20, 2023 12:44

pavelsavara requested review from kg, lewing and radical as code owners March 20, 2023 12:44

maraf reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/mono/wasm/runtime/cwraps.ts Outdated
Comment threadeng/testing/tests.wasm.targets Outdated

maraf approved these changes Mar 20, 2023
View reviewed changes

build-analysisBot mentioned this pull request Mar 20, 2023
Tracking issue for CI build timeouts #76454
Closed

maraf marked this pull request as draft March 21, 2023 20:07

build-analysisBot mentioned this pull request Mar 21, 2023
[wasm] SimpleSourceChangeRebuildTest.SimpleStringChangeInSource - Long running test error, cannot access file used by another process #83752
Closed

vitek-karas reviewed Mar 24, 2023
View reviewed changes

Comment thread...nteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSFunctionBinding.cs Outdated

rebase
8b7ea8c

pavelsavara force-pushed the browser_fix_WasmEnableLegacyJsInterop branch from c8fc59e to 8b7ea8cCompare April 13, 2023 09:38

build-analysisBot mentioned this pull request Apr 13, 2023
[wasm][debugger] DebuggerTests.AssignmentTests.InspectVariableBeforeAndAfterAssignment failing #64188
Closed

pavelsavara added 2 commits April 14, 2023 11:22

Merge branch 'main' into browser_fix_WasmEnableLegacyJsInterop
203111b
# Conflicts: # src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in

fix
a9bf333

build-analysisBot mentioned this pull request Apr 14, 2023
WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434
Closed

pavelsavara marked this pull request as ready for review April 18, 2023 14:33

pavelsavara requested a review from maraf April 18, 2023 14:34

maraf approved these changes Apr 19, 2023
View reviewed changes

pavelsavara merged commit c2663b5 into dotnet:mainApr 19, 2023

ghost locked as resolved and limited conversation to collaborators May 19, 2023

pavelsavara deleted the browser_fix_WasmEnableLegacyJsInterop branch September 2, 2024 15:30

Uh oh!

Conversation

pavelsavara commented Mar 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maraf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavelsavara commented Mar 20, 2023

Uh oh!

pavelsavara commented Mar 20, 2023

Uh oh!

azure-pipelinesBot commented Mar 20, 2023

Uh oh!

amcasey commented Mar 20, 2023

Uh oh!

kg commented Mar 20, 2023

Uh oh!

amcasey commented Mar 20, 2023

Uh oh!

pavelsavara commented Mar 20, 2023

Uh oh!

pavelsavara commented Mar 20, 2023

Uh oh!

maraf commented Mar 21, 2023

Uh oh!

maraf commented Mar 21, 2023

Uh oh!

Uh oh!

vitek-karas commented Mar 24, 2023

Uh oh!

pavelsavara commented Apr 18, 2023

Uh oh!

azure-pipelinesBot commented Apr 18, 2023

Uh oh!

pavelsavara commented Apr 18, 2023

Uh oh!

maraf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pavelsavara commented Mar 20, 2023•
edited
Loading

maraf left a comment •
edited
Loading