-
Notifications
You must be signed in to change notification settings - Fork 332
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
Add wasm opt-in setting #8270
Add wasm opt-in setting #8270
Conversation
packages/devtools_app/lib/src/shared/preferences/preferences.dart
Outdated
Show resolved
Hide resolved
EDIT: resolved this issue. See .gif in PR description above for proof of switching the renderer properly. |
@@ -23,6 +23,9 @@ function unregisterDevToolsServiceWorker() { | |||
|
|||
// Bootstrap app for 3P environments: | |||
function bootstrapAppFor3P() { | |||
const searchParams = new URLSearchParams(window.location.search); | |||
const renderer = searchParams.get('renderer'); | |||
const userConfig = renderer ? {'renderer': renderer} : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you store user settings? Can you read the setting directly from there? This way you don't need to patch the URL. People tend to save and share URLs, so one person's setting could leak to another person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue here is that you're not actually using the userConfig
variable. You probably need to splat it into the config below, i.e.:
...
config: {
canvasKitBaseUrl: 'canvaskit/',
...userConfig,
},
...
Also, if you want JS + CanvasKit by default, you need to set the renderer to canvaskit when there is no url query parameter. The bootstrapper will use the first build it can find that works in the browser environment and abides by the configuration set by the user. The dual-compile build has the dart2wasm+skwasm build as its first option, and dart2js+canvaskit as its second option. If you want to skip the first option, you need to specify that you want canvaskit.
I would actually change the way this url query parameter is structured and just simply make it like allowWasm=true
or something like that. Then, I would specify 'renderer': canvaskit
if that query parameter is not found (or a non-truthy value), and then don't specify the renderer at all if allowWasm=true
. That way, if someone opens it in a browser that doesn't actually support WasmGC or some of the other things skwasm needs, it will still fall back to dart2js+CanvasKit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We store settings on disk in the ~/.flutter-devtools/.devtools
file. DevTools runs on local host, so URLs won't typically be shared unless someone is running on a remote machine and passes that URL around, which is unlikely.
@@ -23,6 +23,9 @@ function unregisterDevToolsServiceWorker() { | |||
|
|||
// Bootstrap app for 3P environments: | |||
function bootstrapAppFor3P() { | |||
const searchParams = new URLSearchParams(window.location.search); | |||
const renderer = searchParams.get('renderer'); | |||
const userConfig = renderer ? {'renderer': renderer} : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue here is that you're not actually using the userConfig
variable. You probably need to splat it into the config below, i.e.:
...
config: {
canvasKitBaseUrl: 'canvaskit/',
...userConfig,
},
...
Also, if you want JS + CanvasKit by default, you need to set the renderer to canvaskit when there is no url query parameter. The bootstrapper will use the first build it can find that works in the browser environment and abides by the configuration set by the user. The dual-compile build has the dart2wasm+skwasm build as its first option, and dart2js+canvaskit as its second option. If you want to skip the first option, you need to specify that you want canvaskit.
I would actually change the way this url query parameter is structured and just simply make it like allowWasm=true
or something like that. Then, I would specify 'renderer': canvaskit
if that query parameter is not found (or a non-truthy value), and then don't specify the renderer at all if allowWasm=true
. That way, if someone opens it in a browser that doesn't actually support WasmGC or some of the other things skwasm needs, it will still fall back to dart2js+CanvasKit.
// TODO(kenz): this may cause an infinite loop of reloading the page if | ||
// the setting from storage or the query parameter indicate we should be | ||
// loading with WASM, but each time we reload the page, something goes wrong | ||
// and we fall back to JS. | ||
toggleWasmEnabled(enabledFromStorage || enabledFromQueryParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyebrowsoffire @yjbanov is there a way to know that we tried to use WASM but had to fall back? Otherwise this could get in an infinite loop when the attempt to load with WASM fails and we fell back to JS.
This would make kIsWasm
false, and we would continually try to change the value of the wasmEnabled
notifier to true based on the value of (enabledFromStorage || enabledFromQueryParams)
. This would then trigger the listener and reload the page again, and repeat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just check the actual query params via JS interop (I'm not sure if DevToolsQueryParams
is actually doing that or if it is some other system for composing the URL to pass back to the user) and then also look at kIsWasm
. If the query parameters in the actual url have the useWasm
parameter, but you're not in wasm mode, then we've fallen back to JS.
I have contemplated exposing some more rich information about what happened in the bootstrap process and why, but I don't know exactly what that would look like and we haven't implemented anything currently.
f91fb31
to
be44066
Compare
@eyebrowsoffire @yjbanov this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think your current strategy prevents the JS fallback. Not sure if that's important to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds a persistent setting to DevTools to opt in to the wasm build. Enabling this setting adds a query parameter to the DevTools URI, which the flutter bootstrap looks for to determine which web renderer to use (following the official guidance). The changes to the flutter bootstrap file are not added behind a flag, so once this PR lands, it will be possible to force wasm by adding the query parameter
wasm=true
(provided that DevTools was compiled with wasm to begin with).Work towards #7856.
The setting is behind a feature flag right now (
FeatureFlags.wasmOptInSetting
), but this flag has been set to true as part of this PR. We will leave the flag for easy disabling of this feature if we need to turn off the wasm-opt-in and we can remove in the future when we are confident that the wasm build of DevTools is stable.