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

Report CompileTime metric in flutter build aot --report-timings. #31895

Conversation

sortie
Copy link
Contributor

@sortie sortie commented May 1, 2019

This is the correct metric to report for compilation time benchmarks rather
than RunTime. Rename the 'gen_snapshot' value to merely 'snapshot' for
backwards compatibility and overall simplicity.

This change simplifies Dart's benchmarking of Flutter by making it easier to
adopt --report-timings (made for Dart to use), which makes the benchmarks
much more robust.

Description

This changes matches what the Dart benchmarking infrastructure needs a little better than the existing behavior. I don't believe anything else is using --report-timings, so this should be fine to slightly change.

Related Issues

The --report-timings flag was introduced in #30032 for use by dart's benchmarking infrastructure.

Tests

I updated the existing test to test the new behavior. I couldn't figure out how to run the test though, cd packages/flutter_tools $ flutter test test/base/build_test.dart failed to me with Failed to load test harness. Are you missing a dependency on flutter_test? and FileSystemException(uri=org-dartlang-untranslatable-uri:package%3Aflutter_test%2Fflutter_test.dart; message=StandardFileSystem only supports file:* and data:* URIs). Let's see if the PR checks work here on github.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@sortie
Copy link
Contributor Author

sortie commented May 1, 2019

@mraleph

packages/flutter_tools/lib/src/base/build.dart Outdated Show resolved Hide resolved
This is the correct metric to report for compilation time benchmarks rather
than RunTime. Rename the 'gen_snapshot' value to merely 'snapshot' for
backwards compatibility and overall simplicity.

This change simplifies Dart's benchmarking of Flutter by making it easier to
adopt --report-timings (made for Dart to use), which makes the benchmarks
much more robust.
@sortie sortie force-pushed the cl-report-compiletime-report-in-flutter-build-aot---report-timings branch from a60bfad to 59e046e Compare May 1, 2019 14:16
@goderbauer goderbauer added the tool Affects the "flutter" command-line tool. See also t: labels. label May 1, 2019
@mraleph mraleph merged commit 8b9eb3e into flutter:master May 2, 2019
@mraleph
Copy link
Member

mraleph commented May 2, 2019

Merged for @sortie because he does not have repository access (AFAIK).

@sortie
Copy link
Contributor Author

sortie commented May 2, 2019

Thanks @mraleph, yeah I didn't. I'll take advantage of this CL then.

@sortie sortie deleted the cl-report-compiletime-report-in-flutter-build-aot---report-timings branch May 2, 2019 13:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants