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

Promote dart2wasm compiler support to the stable browser platform. #2144

Merged
merged 17 commits into from
Dec 5, 2023

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Nov 30, 2023

Closes #2143

I ended up going on quite the cleanup adventure here 🤣 . But I think the result is much cleaner around how it handles --precompiled and the various compilers in the browser platform. We may want to adopt a similar cleanup in other platforms too.

  • Deletes the experimental wasm platform, moves it into the chrome platform (works on chrome stable now).
  • Refactors heavily how the browser platform works, abstracting away the logic for precompiled support, dart2js support, and dart2wasm support into separate classes which implement a new CompilerSupport interface.
  • Drops entirely support for --pub-serve mode. We could bring this back if necessary as another kind of CompilerSupport but I don't think it has any users (let me know if you feel different).
  • Each CompilerSupport instance now gets its own server, browser manager, secret, etc. This resolves complications around knowing which kinds of compiler a given request is asking for, which the previous model couldn't handle well. You will get multiple browser instances if multiple compilers are used.
  • Adds the "precompiled" compiler option. This isn't intended to be passed directly, it is inferred when passing the --precompiled argument. No other compiler options are allowed if it is used (although we could probably if desired?).
  • Dropped isJS and isWasm from Runtime, these no longer make sense. Added extensions to Compiler instead for convenience.

Likely there is some additional consolidation that could happen here, as there is a fair bit of duplicate logic across some of the implementations. But I am not sure if it is really worth it or not.

@jakemac53 jakemac53 marked this pull request as draft November 30, 2023 22:47
@jakemac53 jakemac53 requested a review from kevmoo November 30, 2023 22:47
@jakemac53
Copy link
Contributor Author

jakemac53 commented Nov 30, 2023

TODO: Currently isWasm is tied to the Runtime, but it really shouldn't be. I need to fix how that works so the wasm selector works again. Or, we just require the dart2wasm selector?

Also, how should Runtime.isJs (and the js selector) work in this world? Should it return false if the compiler is dart2wasm? If so, I think we should drop both Runtime.isJs and Runtime.isWasm entirely, and make the selectors be based on the compiler selection. Or, we could move isJs and isWasm to Compiler instead? As far as I can tell these are basically only used for platform selection based on the js and wasm boolean selectors.

I went ahead and refactored things to remove isJS and isWASM from Runtime, and added extensions to Compiler which do the same.

@jakemac53 jakemac53 requested a review from natebosch November 30, 2023 22:54
@kevmoo
Copy link
Member

kevmoo commented Nov 30, 2023

pkg:http works with dart-lang/http#1064 !!!

@kevmoo
Copy link
Member

kevmoo commented Nov 30, 2023

pkg:web_socket_channel at dart-lang/web_socket_channel@df096a9

00:03 +0 -1: [Chrome, Dart2WASM] test/html_test.dart: (setUpAll) [E]
  Type 'double' is not a subtype of type 'int' in type cast
  unparsed  unparsed      at _awaitHelper closure at org-dartlang-sdk:///lib/_internal/wasm/lib/async_patch.dart:82:16 (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[3706]:0xa11ae)
  unparsed  unparsed      at closure wrapper at org-dartlang-sdk:///lib/_internal/wasm/lib/async_patch.dart:82:16 trampoline (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[3709]:0xa1295)
  unparsed  unparsed      at _rootRunUnary (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[1020]:0x57b41)
  unparsed  unparsed      at _rootRunUnary tear-off trampoline (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[4122]:0xaa720)
  unparsed  unparsed      at _CustomZone.runUnary (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[3990]:0xa8766)
  unparsed  unparsed      at _FutureListener.handleValue (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[1014]:0x578df)
  unparsed  unparsed      at _Future._propagateToListeners closure at org-dartlang-sdk:///lib/async/future_impl.dart:836:33 (http://localhost:51661/GPLvykRlNyzVTKHYuiQNTOoWqFnLrW8R/test/html_test.dart.browser_test.dart.wasm:wasm-function[991]:0x5736e)

Both feel like weird double or int issues...obviously...

@jakemac53
Copy link
Contributor Author

@natebosch when you get back if you could provide your thoughts on #2144 (comment) that would be helpful, maybe we should have a quick video chat.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Dec 1, 2023

Another open question here: How should you specify that you want to target precompiled wasm tests?

Maybe I should drop the precompiled compiler notion and allow special --compiler flags still in combination with it. Then the "PrecompiledSupport" class might accept in a compiler as configuration, but it would be chosen not based on the compiler but based on whether a precompiled dir was given. We wouldn't be able to validate necessarily that the correct compiler was actually used reliably, but we don't today anyways.

I also changed this, there is no "precompiled" compiler any more. The existing "precompiled" support will likely get some additional wire-up eventually to do compiler specific things though, if needed.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Dec 1, 2023

I went ahead and dropped Runtime.isJS and Runtime.isWasm, as well as Compiler.precompiled. I think that is indeed the best path forward, but feel free to push back and I can revert it.

I am a bit concerned about the wire format breaking here (for Runtime in particular), this has caused problems in the past with flutter due to flutter/flutter#91107. We might be able to make it more backwards compatible though? I made this backwards compatible (I think?) and pinged the flutter issue.

@jakemac53 jakemac53 marked this pull request as ready for review December 1, 2023 22:02
@natebosch
Copy link
Member

  • Dropped isJS and isWasm from Runtime

We should double check if this impacts flutter usage. Have you tried this in google3 yet? We might want to create a CL for it and run through a presubmit before landing on github.

I made this backwards compatible (I think?) and pinged the flutter issue.

The json protocol is backwards compatible - do we have any Dart API compatibility concerns?

@natebosch
Copy link
Member

I think this all looks good assuming it doesn't cause any issues rolling to flutter or google3 - I'm nervous about dropping support for running tests from an arbitrary server, but I do prefer removing it and seeing if anyone complains over assuming it's working untested.

@jakemac53
Copy link
Contributor Author

I think this all looks good assuming it doesn't cause any issues rolling to flutter or google3

Yes, I definitely plan on validating these changes internally, in the Dart SDK, and in flutter before publishing.

I'm nervous about dropping support for running tests from an arbitrary server, but I do prefer removing it and seeing if anyone complains over assuming it's working untested.

Yeah, I feel like we have kicked the can down the road a lot on this, and I want to see if anybody even notices. We definitely can bring back support if needed, and I will sign up for doing that.

@jakemac53
Copy link
Contributor Author

We should double check if this impacts flutter usage. Have you tried this in google3 yet? We might want to create a CL for it and run through a presubmit before landing on github.

Fwiw my plan was to land it on GitHub, since that makes it easier to test it. But I will wait to publish until validated.

@jakemac53
Copy link
Contributor Author

The json protocol is backwards compatible - do we have any Dart API compatibility concerns?

I did a breaking change in test_api and test_core, as there are some changes to the Runtime and Configuration classes.

Copy link

auto-submit bot commented Dec 5, 2023

auto label is removed for dart-lang/test/2144, due to - The status or check suite unit_test; windows; Dart 3.0.0; PKG: pkgs/test; `dart test --preset travis --total-shards 5 --sha... has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot merged commit e49ae54 into master Dec 5, 2023
1 check passed
@auto-submit auto-submit bot deleted the release-wasm branch December 5, 2023 20:51
@jakemac53 jakemac53 mentioned this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move dart2wasm support to be "normal"
3 participants