[native_assets] Use kernel concatenation #147158

dcharkes · 2024-04-22T11:52:20Z

Uses kernel concatenation to concatenate the program.dill and native_assets.dill.

This will enable compiling native_assets.dill after program.dill so that tree-shaking information can be used.

Issue:

[native_assets] Use kernel concatenation instead of passing in asset-mapping into kernel compilation #146270

Implementation choices

We can either run the frontend_server twice as a process invocation (current PR), or keep the process running after the dart compilation and then shut it down after native assets compilation to kernel. Keeping the process running would require more coordination between different Targets.
We can either chose to only do separate kernel file compilation in release (AOT) builds, or do it both for JIT and AOT (current PR). Doing it differently in JIT/AOT means more conditionals in lib/src/build_system/targets/, doing it single-shot in JIT might be more performant though.

Update: Both of these are mitigated by not running the kernel compiler process if there are no native assets at all.

Implementation notes

This only updates flutter assemble.

The kernel compilation calls in flutter run do not require kernel concatenation. The native-assets-kernel-mapping in flutter run contains results from --dry-run invocations of hook/build.dart (and in the future hook/link.dart). These --dry-run invocations do not get access to tree-shaking information, so the link hook to be introduced later can be run before Dart compilation.

Tests

Updated the Targets tests to reflect the new implementation.

Related CLs

frontend_server support: https://dart-review.googlesource.com/c/sdk/+/363567
Same PR for Dart standalone: https://dart-review.googlesource.com/c/sdk/+/362384

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

flutter-dashboard · 2024-04-22T11:52:23Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dcharkes · 2024-04-22T11:56:08Z

@christopherfujino @stuartmorgan

There are some implementation considerations for this PR.

We can either run the frontend_server twice as a process invocation (current PR), or keep the process running after the dart compilation and then shut it down after native assets compilation to kernel. Keeping the process running would require more coordination between different Targets.
We can either chose to only do separate kernel file compilation in release (AOT) builds, or do it both for JIT and AOT (current PR). Doing it differently in JIT/AOT means more conditionals in lib/src/build_system/targets/, doing it single-shot in JIT might be more performant though.

I've taken the approach with the smallest complexity and smallest diff for now. Do you think we should incur the increased complexity for more speedy builds?

dcharkes · 2024-04-22T12:51:30Z

FYI @mosuem

christopherfujino · 2024-04-23T16:54:40Z

packages/flutter_tools/lib/src/build_system/targets/common.dart

 /// even though it is not listed as an input. Pub inserts a timestamp into
 /// the file which causes unnecessary rebuilds, so instead a subset of the contents
 /// are used an input instead.
-class KernelSnapshot extends Target {


Can you update the dartdoc to explain its relationship to KernelSnapshot.

christopherfujino · 2024-04-23T17:33:29Z

packages/flutter_tools/lib/src/build_system/targets/common.dart

+
+    final String dillPath = environment.buildDir.childFile(dillName).path;
+
+    final CompilerOutput? output = await compiler.compile(


Could we skip the compile if the user doesn't have native assets?

christopherfujino

LGTM!

dcharkes · 2024-04-23T19:09:25Z

Thanks @christopherfujino !

auto-submit · 2024-04-23T20:55:28Z

auto label is removed for flutter/flutter/147158, due to - The status or check suite Windows module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2024-04-24T09:03:26Z

auto label is removed for flutter/flutter/147158, due to - The status or check suite Windows module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

)

flutter/flutter@dba4f77...5d3bca4 2024-04-25 [email protected] Roll Flutter Engine from 6b33890a4701 to 674890ce7141 (1 revision) (flutter/flutter#147365) 2024-04-25 [email protected] [native_assets] Use kernel concatenation (flutter/flutter#147158) 2024-04-25 [email protected] Roll Flutter Engine from ccd57f5a4bc4 to 6b33890a4701 (2 revisions) (flutter/flutter#147359) 2024-04-25 [email protected] Roll Flutter Engine from 9de649ed2926 to ccd57f5a4bc4 (1 revision) (flutter/flutter#147356) 2024-04-25 [email protected] Roll Flutter Engine from 2f48456a425a to 9de649ed2926 (2 revisions) (flutter/flutter#147354) 2024-04-25 [email protected] Roll Flutter Engine from f48f3b6a0172 to 2f48456a425a (1 revision) (flutter/flutter#147351) 2024-04-25 [email protected] Roll Flutter Engine from b30c0a765680 to f48f3b6a0172 (8 revisions) (flutter/flutter#147348) 2024-04-25 [email protected] Allow the SceneBuilder, PictureRecord, and Canvas constructor calls from the rendering layer to be hooked (flutter/flutter#147271) 2024-04-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.3 to 4.1.4 (flutter/flutter#147334) 2024-04-24 [email protected] Fix filled color is wrong for a focused and hovered TextField (flutter/flutter#146976) 2024-04-24 [email protected] Roll Flutter Engine from b5d5832f7144 to b30c0a765680 (14 revisions) (flutter/flutter#147336) 2024-04-24 [email protected] Run new_gallery__transition_perf on mokey in staging (flutter/flutter#147339) 2024-04-24 [email protected] Remove hidden dependencies on the default goldenFileComparator. (flutter/flutter#146956) 2024-04-24 [email protected] Add create app and plugin templates for Swift Package Manager (flutter/flutter#147082) 2024-04-24 [email protected] Add support for overriding `reverseCurve` with `ExpansionTile.expansionAnimationStyle` (flutter/flutter#147103) 2024-04-24 [email protected] Mark firebase tests as `bringup: true` (flutter/flutter#147338) 2024-04-24 [email protected] Add Valentin Vignal to AUTHORS (flutter/flutter#147314) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

)

[native_assets] Use kernel concatenation

f182f7c

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 22, 2024

dcharkes added 4 commits April 22, 2024 14:33

FIx common_test.dart

e8ee6f5

Fix formatting

709268f

Use github handle

9468ee9

formatting

3336c8e

dcharkes requested review from christopherfujino and stuartmorgan-g April 22, 2024 12:54

christopherfujino reviewed Apr 23, 2024

View reviewed changes

dcharkes added 2 commits April 23, 2024 20:17

Merge branch 'master' into native-assets-kernel-concat

1be0fee

Skip compiling native_assets.yaml to kernel if empty

e330143

christopherfujino approved these changes Apr 23, 2024

View reviewed changes

fix formatting

e249c39

dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 23, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 23, 2024

dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 24, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 24, 2024

dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 25, 2024

auto-submit bot merged commit e410b7c into master Apr 25, 2024

auto-submit bot deleted the native-assets-kernel-concat branch April 25, 2024 11:22

engine-flutter-autoroll mentioned this pull request Apr 25, 2024

Roll Flutter from dba4f7747441 to 5d3bca4500b1 (17 revisions) flutter/packages#6612

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 25, 2024

e410b7c [native_assets] Use kernel concatenation (flutter/flutter#147158

b3673a0

)

knopp mentioned this pull request May 1, 2024

Kernel snapshot input dependency tracking not working #147643

Closed

dcharkes mentioned this pull request May 8, 2024

Regression in build time esp. on Windows #147982

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

e410b7c [native_assets] Use kernel concatenation (flutter/flutter#147158

b9b59b2

)

dcharkes mentioned this pull request Sep 24, 2024

[native_assets] Stop doing dry run in flutter run #154425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[native_assets] Use kernel concatenation #147158

[native_assets] Use kernel concatenation #147158

Uh oh!

dcharkes commented Apr 22, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Apr 22, 2024

Uh oh!

dcharkes commented Apr 22, 2024

Uh oh!

dcharkes commented Apr 22, 2024

Uh oh!

christopherfujino Apr 23, 2024

Uh oh!

christopherfujino Apr 23, 2024

Uh oh!

christopherfujino left a comment

Uh oh!

dcharkes commented Apr 23, 2024

Uh oh!

auto-submit bot commented Apr 23, 2024

Uh oh!

auto-submit bot commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		final String dillPath = environment.buildDir.childFile(dillName).path;

		final CompilerOutput? output = await compiler.compile(

[native_assets] Use kernel concatenation #147158

[native_assets] Use kernel concatenation #147158

Uh oh!

Conversation

dcharkes commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation choices

Implementation notes

Tests

Related CLs

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 22, 2024

Uh oh!

dcharkes commented Apr 22, 2024

Uh oh!

dcharkes commented Apr 22, 2024

Uh oh!

christopherfujino Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

dcharkes commented Apr 23, 2024

Uh oh!

auto-submit bot commented Apr 23, 2024

Uh oh!

auto-submit bot commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dcharkes commented Apr 22, 2024 •

edited

Loading