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.

Sign up for GitHub

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

Jump to bottom

[browser] fix trimming of LegacyExports with WasmEnableLegacyJsInterop #83664

Merged
merged 3 commits into from
Apr 19, 2023
Merged

[browser] fix trimming of LegacyExports with WasmEnableLegacyJsInterop #83664

merged 3 commits into from
Apr 19, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Mar 20, 2023 •
edited
Loading

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

@pavelsavara pavelsavara added this to the 8.0.0 milestone Mar 20, 2023
@pavelsavara pavelsavara requested a review from maraf March 20, 2023 10:15
@pavelsavara pavelsavara self-assigned this Mar 20, 2023
@ghost
Copy link

ghost commented Mar 20, 2023

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

This was referenced Mar 20, 2023
Merged
Merged
@pavelsavara pavelsavara marked this pull request as ready for review March 20, 2023 12:44
maraf
maraf reviewed Mar 20, 2023
View reviewed changes
src/mono/wasm/runtime/cwraps.ts Outdated Show resolved Hide resolved
eng/testing/tests.wasm.targets Outdated Show resolved Hide resolved
maraf
maraf approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@maraf maraf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My questions above are non-blocking

@build-analysis build-analysis bot mentioned this pull request Mar 20, 2023
Closed
@pavelsavara
Copy link
Member Author

@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
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@amcasey
Copy link
Member

amcasey commented Mar 20, 2023

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

@kg
Copy link
Contributor

kg commented Mar 20, 2023

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
Copy link
Member

amcasey commented Mar 20, 2023

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
Copy link
Member Author

@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
Copy link
Member Author

@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

This was referenced Mar 21, 2023
Closed
Closed
Closed
Closed
@maraf
Copy link
Member

maraf commented Mar 21, 2023

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 maraf marked this pull request as draft March 21, 2023 20:07
@maraf
Copy link
Member

maraf commented Mar 21, 2023

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

@build-analysis build-analysis bot mentioned this pull request Mar 21, 2023
Closed
@vitek-karas
Copy link
Member

@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 :-).

@pavelsavara pavelsavara force-pushed the browser_fix_WasmEnableLegacyJsInterop branch from c8fc59e to 8b7ea8c Compare April 13, 2023 09:38
@build-analysis build-analysis bot mentioned this pull request Apr 13, 2023
Closed
@pavelsavara
Merge branch 'main' into browser_fix_WasmEnableLegacyJsInterop
203111b 
# Conflicts:
#	src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in
@pavelsavara
fix
a9bf333
@build-analysis build-analysis bot mentioned this pull request Apr 14, 2023
Closed
@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

CI failures are unrelated

@pavelsavara pavelsavara marked this pull request as ready for review April 18, 2023 14:33
@pavelsavara pavelsavara requested a review from maraf April 18, 2023 14:34
This was referenced Apr 18, 2023
Closed
Closed
Closed
maraf
maraf approved these changes Apr 19, 2023
View reviewed changes
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 pavelsavara merged commit c2663b5 into dotnet:main Apr 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2023
@pavelsavara pavelsavara deleted the browser_fix_WasmEnableLegacyJsInterop branch September 2, 2024 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vitek-karas vitek-karas vitek-karas left review comments

@maraf maraf maraf approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@radical radical Awaiting requested review from radical

@kg kg Awaiting requested review from kg

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pavelsavara @amcasey @kg @maraf @vitek-karas