Skip to content

[flutter_tools] refactor GenSnapshot and AotBuilder#52091

Merged
jonahwilliams merged 9 commits intoflutter:masterfrom
jonahwilliams:output_heap_snapshot
Mar 19, 2020
Merged

[flutter_tools] refactor GenSnapshot and AotBuilder#52091
jonahwilliams merged 9 commits intoflutter:masterfrom
jonahwilliams:output_heap_snapshot

Conversation

@jonahwilliams
Copy link
Copy Markdown
Contributor

@jonahwilliams jonahwilliams commented Mar 6, 2020 •
edited
Loading

Description

  • Remove dependency injection from GenSnapshot and AotBuilder.
  • Update tests to use FakeProcessManager
  • Delete duplicate AOT build code in aot.dart and replace with assemble, since we can't get rid of the command for now.
  • Delete duplicate kernel compilation code in GenSnapshot

Upon refactoring test code, discover that the LocalEngineArtifacts._engineSrcPath and Artifact.snapshotDart are completely unused (might predate gen snapshot?). Remove and refactor.

@fluttergithubbot fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Mar 6, 2020
@jonahwilliams jonahwilliams marked this pull request as ready for review March 6, 2020 20:46
@zanderso zanderso self-requested a review March 6, 2020 21:42
@Piinks
Copy link
Copy Markdown
Contributor

Piinks commented Mar 6, 2020

Same here: The flutter-gold check can be disregarded here. I just made a change in flutter/cocoon for it to ignore changes that don't run golden file tests, so it won't be coming back around to update this one.

@jonahwilliams
Copy link
Copy Markdown
Contributor Author

jonahwilliams commented Mar 18, 2020

I pulled out the snapshot.dart removal here: https://www.google.com/url?sa=j&url=https%3A%2F%2Fgithub.com%2Fflutter%2Fflutter%2Fpull%2F52821%2F&uct=1579727840&usg=IRCnY8nPB2vFdG6i0kqqpgYGubo.&source=chat

zanderso
zanderso reviewed Mar 18, 2020
View reviewed changes
Comment thread packages/flutter_tools/lib/src/aot.dart Outdated
case TargetPlatform.darwin_x64:
case TargetPlatform.linux_x64:
case TargetPlatform.windows_x64:
case TargetPlatform.fuchsia_arm64:
Copy link
Copy Markdown
Member

@zanderso zanderso Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fuchsia supports AOT, but not via this path. Maybe some more targeted tool exit message for fuchsia would be good.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread packages/flutter_tools/lib/src/aot.dart Outdated
status?.cancel();
globals.printError(error.toString());
return;
throwToolExit(null);
Copy link
Copy Markdown
Member

@zanderso zanderso Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A message here would probably be helpful, even if it was just something like "The AOT build failed".

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread packages/flutter_tools/lib/src/base/build.dart

final SnapshotType snapshotType = SnapshotType(platform, buildMode);
final int genSnapshotExitCode =
await _timedStep('snapshot(CompileTime)', 'aot-snapshot',
Copy link
Copy Markdown
Member

@zanderso zanderso Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this timing info still captured some other way? Or is this reporting no longer valuable because we expect users to be taking a different path?

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use a _timedStep for the current AOT build.

All of this timing information is captured via PerformanceMeasurement classes. This isn't hooked up to analytics yet, but I could update this PR to do so. This has the benefit of allowing uniform handling of build performance

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/build_system/build_system.dart#L619

I could wire this up in this PR or do it separately

Copy link
Copy Markdown
Member

@zanderso zanderso Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately sgtm.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filled #52842 to track

Copy link
Copy Markdown
Contributor

@whesse whesse Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this _timedStep removed the compilation times from the log output reported using the --report-timings flag in Golem benchmarking of Flutter.

The failure was not noticed, because the previous backup code path that was used before the --report-timings flag was introduced started running again.

zanderso
zanderso approved these changes Mar 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm w/ small fix

Comment thread packages/flutter_tools/lib/src/aot.dart Outdated
Copy link
Copy Markdown
Member

@zanderso zanderso Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also TargetPlatform.fuchsia_arm64

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks!

@jonahwilliams jonahwilliams merged commit f65421a into flutter:master Mar 19, 2020
@jonahwilliams jonahwilliams deleted the output_heap_snapshot branch March 19, 2020 16:36
jonahwilliams pushed a commit that referenced this pull request Mar 19, 2020
Revert "[flutter_tools] refactor GenSnapshot and AotBuilder (#52091)"
d3832e4 
This reverts commit f65421a.
@jonahwilliams jonahwilliams mentioned this pull request Mar 19, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Mar 19, 2020
Revert "[flutter_tools] refactor GenSnapshot and AotBuilder (#52091)" (…
9391e48 
…#52893)

This reverts commit f65421a.
@jonahwilliams jonahwilliams mentioned this pull request Mar 19, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Mar 19, 2020
[flutter_tools] refactor AotBuilder, removing globals and duplicated …
b21e780 
…kernel compilation (#52896)

Reland of #52091
@lock
Copy link
Copy Markdown

lock Bot commented Apr 2, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@lock lock Bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@whesse whesse whesse left review comments

@zanderso zanderso zanderso approved these changes

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonahwilliams @Piinks @whesse @zanderso @googlebot @fluttergithubbot