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 SkiaPerfPoint and FlutterEngineMetricPoint #70153
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright 2014 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
import 'package:metrics_center/src/common.dart'; | ||
|
||
/// Convenient class to capture the benchmarks in the Flutter engine repo. | ||
class FlutterEngineMetricPoint extends MetricPoint { | ||
FlutterEngineMetricPoint( | ||
String name, | ||
double value, | ||
String gitRevision, { | ||
Map<String, String> moreTags = const <String, String>{}, | ||
}) : super( | ||
value, | ||
<String, String>{ | ||
kNameKey: name, | ||
kGithubRepoKey: kFlutterEngineRepo, | ||
kGitRevisionKey: gitRevision, | ||
}..addAll(moreTags), | ||
); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
// Copyright 2014 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// This file uses Dart 2.12 semantics. This is needed as we can't upgrade | ||
// the SDK constraint to `>=2.12.0-0` before the deps are ready. | ||
// @dart=2.12 | ||
|
||
import 'package:github/github.dart'; | ||
|
||
/// Singleton class to query some Github info with an in-memory cache. | ||
class GithubHelper { | ||
/// Return the singleton helper. | ||
factory GithubHelper() { | ||
return _singleton; | ||
} | ||
|
||
GithubHelper._internal(); | ||
|
||
/// The result is cached in memory so querying the same thing again in the | ||
/// same process is fast. | ||
Comment on lines
+20
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the test, 1000 queries per second is needed. Shall the doc reflect that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, added. |
||
/// | ||
/// Our unit test requires that calling this method 1000 times for the same | ||
/// `githubRepo` and `sha` should be done in 1 second. | ||
Future<DateTime> getCommitDateTime(String githubRepo, String sha) async { | ||
final String key = '$githubRepo/commit/$sha'; | ||
if (_commitDateTimeCache[key] == null) { | ||
final RepositoryCommit commit = await _github.repositories | ||
.getCommit(RepositorySlug.full(githubRepo), sha); | ||
_commitDateTimeCache[key] = commit.commit.committer.date; | ||
} | ||
return _commitDateTimeCache[key]!; | ||
} | ||
|
||
static final GithubHelper _singleton = GithubHelper._internal(); | ||
|
||
final GitHub _github = GitHub(); | ||
final Map<String, DateTime> _commitDateTimeCache = <String, DateTime>{}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
// Copyright 2014 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// This file uses Dart 2.12 semantics. This is needed as we can't upgrade | ||
// the SDK constraint to `>=2.12.0-0` before the deps are ready. | ||
// @dart=2.12 | ||
|
||
import 'package:metrics_center/src/common.dart'; | ||
|
||
// Skia Perf Format is a JSON file that looks like: | ||
|
||
// { | ||
// "gitHash": "fe4a4029a080bc955e9588d05a6cd9eb490845d4", | ||
// "key": { | ||
// "arch": "x86", | ||
// "gpu": "GTX660", | ||
// "model": "ShuttleA", | ||
// "os": "Ubuntu12" | ||
// }, | ||
// "results": { | ||
// "ChunkAlloc_PushPop_640_480": { | ||
// "nonrendering": { | ||
// "min_ms": 0.01485466666666667, | ||
// "options": { | ||
// "source_type": "bench" | ||
// } | ||
// } | ||
// }, | ||
// "DeferredSurfaceCopy_discardable_640_480": { | ||
// "565": { | ||
// "min_ms": 2.215988, | ||
// "options": { | ||
// "source_type": "bench" | ||
// } | ||
// }, | ||
// ... | ||
|
||
class SkiaPerfPoint extends MetricPoint { | ||
SkiaPerfPoint._(this.githubRepo, this.gitHash, this.testName, this.subResult, | ||
double value, this._options, this.jsonUrl) | ||
: assert(_options[kGithubRepoKey] == null), | ||
assert(_options[kGitRevisionKey] == null), | ||
assert(_options[kNameKey] == null), | ||
super( | ||
value, | ||
<String, String>{} | ||
..addAll(_options) | ||
..addAll(<String, String>{ | ||
kGithubRepoKey: githubRepo, | ||
kGitRevisionKey: gitHash, | ||
kNameKey: testName, | ||
kSubResultKey: subResult, | ||
}), | ||
) { | ||
assert(tags[kGithubRepoKey] != null); | ||
assert(tags[kGitRevisionKey] != null); | ||
assert(tags[kNameKey] != null); | ||
} | ||
|
||
/// Construct [SkiaPerfPoint] from a well-formed [MetricPoint]. | ||
/// | ||
/// The [MetricPoint] must have [kGithubRepoKey], [kGitRevisionKey], | ||
/// [kNameKey] in its tags for this to be successful. | ||
/// | ||
/// If the [MetricPoint] has a tag 'date', that tag will be removed so Skia | ||
/// perf can plot multiple metrics with different date as a single trace. | ||
/// Skia perf will use the git revision's date instead of this date tag in | ||
/// the time axis. | ||
factory SkiaPerfPoint.fromPoint(MetricPoint p) { | ||
final String githubRepo = p.tags[kGithubRepoKey]!; | ||
final String gitHash = p.tags[kGitRevisionKey]!; | ||
final String name = p.tags[kNameKey]!; | ||
final String subResult = p.tags[kSubResultKey] ?? kSkiaPerfValueKey; | ||
|
||
final Map<String, String> options = <String, String>{}..addEntries( | ||
p.tags.entries.where( | ||
(MapEntry<String, dynamic> entry) => | ||
entry.key != kGithubRepoKey && | ||
entry.key != kGitRevisionKey && | ||
entry.key != kNameKey && | ||
entry.key != kSubResultKey && | ||
// https://github.com/google/benchmark automatically generates a | ||
// 'date' field. If it's included in options, the Skia perf won't | ||
// be able to connect different points in a single trace because | ||
// the date is always different. | ||
entry.key != 'date', | ||
), | ||
); | ||
|
||
return SkiaPerfPoint._( | ||
githubRepo, gitHash, name, subResult, p.value, options, null); | ||
} | ||
|
||
/// In the format of '<owner>/<name>' such as 'flutter/flutter' or | ||
/// 'flutter/engine'. | ||
final String githubRepo; | ||
|
||
/// SHA such as 'ad20d368ffa09559754e4b2b5c12951341ca3b2d' | ||
final String gitHash; | ||
|
||
/// For Flutter devicelab, this is the task name (e.g., | ||
/// 'flutter_gallery__transition_perf'); for Google benchmark, this is the | ||
/// benchmark name (e.g., 'BM_ShellShutdown'). | ||
/// | ||
/// In Skia perf web dashboard, this value can be queried and filtered by | ||
/// "test". | ||
final String testName; | ||
|
||
/// The name of "subResult" comes from the special treatment of "sub_result" | ||
/// in SkiaPerf. If not provided, its value will be set to kSkiaPerfValueKey. | ||
/// | ||
/// When Google benchmarks are converted to SkiaPerfPoint, this subResult | ||
/// could be "cpu_time" or "real_time". | ||
/// | ||
/// When devicelab benchmarks are converted to SkiaPerfPoint, this subResult | ||
/// is often the metric name such as "average_frame_build_time_millis" whereas | ||
/// the [testName] is the benchmark or task name such as | ||
/// "flutter_gallery__transition_perf". | ||
final String subResult; | ||
|
||
/// The url to the Skia perf json file in the Google Cloud Storage bucket. | ||
/// | ||
/// This can be null if the point has been stored in the bucket yet. | ||
final String? jsonUrl; | ||
|
||
Map<String, dynamic> _toSubResultJson() { | ||
return <String, dynamic>{ | ||
subResult: value, | ||
kSkiaPerfOptionsKey: _options, | ||
}; | ||
} | ||
|
||
/// Convert a list of SkiaPoints with the same git repo and git revision into | ||
/// a single json file in the Skia perf format. | ||
/// | ||
/// The list must be non-empty. | ||
static Map<String, dynamic> toSkiaPerfJson(List<SkiaPerfPoint> points) { | ||
assert(points.isNotEmpty); | ||
assert(() { | ||
for (final SkiaPerfPoint p in points) { | ||
if (p.githubRepo != points[0].githubRepo || | ||
p.gitHash != points[0].gitHash) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
}(), 'All points must have same githubRepo and gitHash'); | ||
|
||
final Map<String, dynamic> results = <String, dynamic>{}; | ||
for (final SkiaPerfPoint p in points) { | ||
final Map<String, dynamic> subResultJson = p._toSubResultJson(); | ||
if (results[p.testName] == null) { | ||
results[p.testName] = <String, dynamic>{ | ||
kSkiaPerfDefaultConfig: subResultJson, | ||
}; | ||
} else { | ||
// Flutter currently doesn't support having the same name but different | ||
// options/configurations. If this actually happens in the future, we | ||
// probably can use different values of config (currently there's only | ||
// one kSkiaPerfDefaultConfig) to resolve the conflict. | ||
assert(results[p.testName][kSkiaPerfDefaultConfig][kSkiaPerfOptionsKey] | ||
.toString() == | ||
subResultJson[kSkiaPerfOptionsKey].toString()); | ||
assert( | ||
results[p.testName][kSkiaPerfDefaultConfig][p.subResult] == null); | ||
results[p.testName][kSkiaPerfDefaultConfig][p.subResult] = p.value; | ||
} | ||
} | ||
|
||
return <String, dynamic>{ | ||
kSkiaPerfGitHashKey: points[0].gitHash, | ||
kSkiaPerfResultsKey: results, | ||
}; | ||
} | ||
|
||
// Equivalent to tags without git repo, git hash, and name because those two | ||
// are already stored somewhere else. | ||
final Map<String, String> _options; | ||
} | ||
|
||
const String kSkiaPerfGitHashKey = 'gitHash'; | ||
const String kSkiaPerfResultsKey = 'results'; | ||
const String kSkiaPerfValueKey = 'value'; | ||
const String kSkiaPerfOptionsKey = 'options'; | ||
const String kSkiaPerfDefaultConfig = 'default'; |
41 changes: 41 additions & 0 deletions
41
dev/benchmarks/metrics_center/test/github_helper_test.dart
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright 2014 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
@Timeout(Duration(seconds: 3600)) | ||
|
||
import 'package:metrics_center/src/github_helper.dart'; | ||
|
||
import 'common.dart'; | ||
|
||
void main() { | ||
test('GithubHelper gets correct commit date time', () async { | ||
final GithubHelper helper = GithubHelper(); | ||
expect( | ||
await helper.getCommitDateTime( | ||
'flutter/flutter', | ||
'ad20d368ffa09559754e4b2b5c12951341ca3b2d', | ||
), | ||
equals(DateTime.parse('2019-12-06 03:33:01.000Z')), | ||
); | ||
}); | ||
|
||
test('GithubHelper is a singleton', () { | ||
final GithubHelper helper1 = GithubHelper(); | ||
final GithubHelper helper2 = GithubHelper(); | ||
expect(helper1, equals(helper2)); | ||
}); | ||
|
||
test('GithubHelper can query the same commit 1000 times within 1 second', | ||
() async { | ||
final DateTime start = DateTime.now(); | ||
for (int i = 0; i < 1000; i += 1) { | ||
await GithubHelper().getCommitDateTime( | ||
'flutter/flutter', | ||
'ad20d368ffa09559754e4b2b5c12951341ca3b2d', | ||
); | ||
} | ||
final Duration duration = DateTime.now().difference(start); | ||
expect(duration, lessThan(const Duration(seconds: 1))); | ||
}); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
a file named flutter_engine.dart seems more clear.
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'm planning to add more Flutter-related but non-engine-related classes such as
FlutterDestination
toflutter.dart
. I can also create aflutter
directory and putflutter_engine.dart
and other files inside it. For now, I don't see many files or content to be needed so I adopted a singleflutter.dart
for now. I'm happy to change either now or in the future if you still prefer it considering the upcoming changes.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.
Thanks for the explanation. Then it should be fine to use flutter.