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

Avoid relying on unevaluated constants (dart defines) in the web engine #115853

Open
eyebrowsoffire opened this issue Nov 22, 2022 · 2 comments
Open
Labels
c: proposal A detailed proposal for a change to Flutter engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team

Comments

@eyebrowsoffire
Copy link
Contributor

eyebrowsoffire commented Nov 22, 2022

We are in the middle of switching over to using pre-built platform dill files when invoking dart2js, see #99161. However, it appears that dart2js does not support unevaluated constants in the platform dill file. This means that the value of any dart defines that we use in the engine must be decided at the time that we build the platform kernels.

Here is my suggested plan of action on how to achieve this:

  1. For the dart defines that decide which renderer we use, we will build separate sets of platform dills for each renderer variation and consume the corresponding dill during the app compilation process. We are already producing the renderer-specific files from the engine, and there are some tentative changes to adopt those new platform dills when invoking dart2js: Platform binaries reland #115502

  2. All other dart defines that we use should be specifiable via the FlutterConfiguration object. Most of them already are, but there are a few that aren't, and we should convert those to have a corresponding flag in the FlutterConfiguration object.

  3. In order to continue supporting use-cases where these are specified via dart defines, modify the dart wrapper that the flutter tool creates for the application to take the dart defines and pass them in via the dart configuration. Since the dart wrapper is compiled as part of the application compile process (and not part of the platform dill build) it will respect the dart defines that are passed in.

  4. After this has changed, we can remove the code that reads the other dart defines in the engine and rely entirely on the flutter configuration for this.

@eyebrowsoffire
Copy link
Contributor Author

Here is a breakdown of the current dart defines being used by the engine:

dart.vm.product and dart.vm.profile

  • From what I can tell, these are only used in the web engine to put a flt-build-mode attribute on the dom body. Certainly not benefitting from tree-shaking, but it seems like it would be weird to inject this via a configuration flag. Perhaps instead we should just have a build mode string that comes into the flutter configuration.

FLUTTER_WEB_AUTO_DETECT, FLUTTER_WEB_USE_SKWASM, FLUTTER_WEB_USE_SKIA

  • These are the renderer defines, we will keep them and make different platform dill variants for these

FLUTTER_WEB_CANVASKIT_URL, FLUTTER_WEB_CANVASKIT_FORCE_CPU_ONLY, FLUTTER_WEB_MAXIMUM_SURFACES, FLUTTER_WEB_DEBUG_SHOW_SEMANTICS

  • These already have configuration flags that can be used instead of these.

FLUTTER_WEB_ENABLE_PROFILING and FLUTTER_WEB_ENABLE_INSTRUMENTATION

  • This is used to see if we should emit certain events to be picked up by the benchmarks. Shouldn't have a significant difference to binary size/tree shaking. We should make this tweakable via configuration flags

BROWSER_IMAGE_DECODING_ENABLED

  • The comments on this say that maybe this should just be removed once we're confident in this feature. It seems like you could tree shake some of the code based on this being false though... although that doesn't really seem to happen in practice at all. We should either just straight up remove this define, or make it tweakable via configuration flags.

flutter.canvaskit.msaa

  • This just becomes a runtime boolean that is passed into skia. We should specify this via configuration flags.

@joshualitt
Copy link
Contributor

@sigmundch for FYI, this is a great analysis. One minor point, dart2js does support unevaluated constants in the platform dill file. What isn't supported currently in the CFE is defining some environment variables, while expecting others to be unevaluated. Only either / or is supported, all environment variables unevaluated or all environment variables evaluated.

@danagbemava-nc danagbemava-nc added in triage Presently being triaged by the triage team engine flutter/engine repository. See also e: labels. platform-web Web applications specifically c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels Nov 23, 2022
@yjbanov yjbanov added the P2 Important issues not at the top of the work list label Dec 1, 2022
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-web Owned by Web platform team triaged-web Triaged by Web platform team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: proposal A detailed proposal for a change to Flutter engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team
Projects
None yet
Development

No branches or pull requests

5 participants