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

[CP] [dart2wasm] Pass source maps to wasm-opt when optimizing #56423

Closed
osa1 opened this issue Aug 9, 2024 · 5 comments
Closed

[CP] [dart2wasm] Pass source maps to wasm-opt when optimizing #56423

osa1 opened this issue Aug 9, 2024 · 5 comments
Assignees
@vsmenon @mkustermann @itsjustkevin @mit-mit @athomas
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve

Comments

@osa1
Copy link
Member

osa1 commented Aug 9, 2024 •
edited
Loading

Commit(s) to merge

1b1740e
075c443

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/379782

Issue Description

dart2wasm currently does not pass the generated source map to wasm-opt when optimizing with wasm-opt.

As a result the source map becomes invalid when the optimizations are enabled (i.e. always when -O0 is not used).

What is the fix

With this commit we pass the source map file to wasm-opt, which then updates it as it transforms the code.

Why cherry-pick

Source map support in dart2wasm was cherry-picked to stable in #56239, but they are useless when optimizations are enabled.

Risk

Assuming good test coverage of wasm-opt, I don't expect this change to break anything.

Issue link(s)

We don't have a GitHub issue for this issue.

Extra Info

No response
@osa1 osa1 added the cherry-pick-review Issue that need cherry pick triage to approve label Aug 9, 2024
@osa1 osa1 added the area-dart2wasm Issues for the dart2wasm compiler. label Aug 9, 2024
@kevmoo kevmoo mentioned this issue Aug 9, 2024
Closed
7 tasks
@mkustermann
Copy link
Member

mkustermann commented Aug 9, 2024 •
edited
Loading

LGTM

@itsjustkevin any chance we could get this approved & landed to be included in time for upcoming dot release?

@jason-simmons
Copy link
Contributor

Flutter is seeing a regression with 1b1740e

Flutter's tooling is passing the --extra-compiler-option=--no-source-maps flag. With that flag I'm seeing an exception:

PathNotFoundException: Cannot rename file to '.../.dart_tool/flutter_build/0c09e3329b6c595a7b1c1f0a8ae9b18f/main.dart.unopt.wasm.map', path = '.../.dart_tool/flutter_build/0c09e3329b6c595a7b1c1f0a8ae9b18f/main.dart.wasm.map' (OS Error: No such file or directory, errno = 2)
#0      _File.throwIfError (dart:io/file_impl.dart:675:7)
#1      _File.renameSync (dart:io/file_impl.dart:349:5)
#2      CompileWasmCommand.run (package:dartdev/src/commands/compile.dart:860:33)
<asynchronous suspension>
#3      CommandRunner.runCommand (package:args/command_runner.dart:212:13)
<asynchronous suspension>
#4      DartdevRunner.runCommand (package:dartdev/dartdev.dart:236:18)
<asynchronous suspension>
#5      runDartdev (package:dartdev/dartdev.dart:47:16)
<asynchronous suspension>
#6      main (file:///usr/local/google/home/jsimmons/sky/engine/src/flutter/third_party/dart/pkg/dartdev/bin/dartdev.dart:13:5)

@osa1
Copy link
Member Author

osa1 commented Aug 12, 2024 •
edited
Loading

The issue above should be fixed with 075c443.

I've added that commit to my cherry-pick CL: https://dart-review.googlesource.com/c/sdk/+/379782.

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Aug 12, 2024
@itsjustkevin
Copy link
Contributor

@osa1 approved, please merge

copybara-service bot pushed a commit that referenced this issue Aug 12, 2024
@osa1 @mkustermann
[stable][dart2wasm] Pass source maps to wasm-opt when optimizing
4ca4d60 
To be able to know when we are generating a source map, make `dart
compile wasm` aware of the `--no-source-maps` flag.

The "name" segments of source mappings are also made `null` with this
patch. Browsers don't use that segment and binaryen doesn't support it.

Change-Id: I7b52c8fb7cef92ed60547e97ad137e0cd3967f26
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/378421
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/380000
Cherry-pick-request: #56423
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/379782
Reviewed-by: Martin Kustermann <kustermann@google.com>
@mkustermann
Copy link
Member

@osa1 approved, please merge

The CL was merged.

@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@mkustermann mkustermann

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@osa1 @vsmenon @mkustermann @itsjustkevin @mit-mit @jason-simmons @athomas