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
Conversation
99254a5
to
372ba8b
Compare
372ba8b
to
ae06a7e
Compare
Unit tests that cover their translations are also added.
ae06a7e
to
ef9d038
Compare
import 'package:metrics_center/src/common.dart'; | ||
|
||
/// Convenient class to capture the benchmarks in the Flutter engine repo. | ||
class FlutterEngineMetricPoint extends MetricPoint { |
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
to flutter.dart
. I can also create a flutter
directory and put flutter_engine.dart
and other files inside it. For now, I don't see many files or content to be needed so I adopted a single flutter.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.
const String kSkiaPerfResultsKey = 'results'; | ||
const String kSkiaPerfValueKey = 'value'; | ||
const String kSkiaPerfOptionsKey = 'options'; | ||
|
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.
Nit: remove.
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.
Done.
/// The result is cached in memory so querying the same thing again in the | ||
/// same process is fast. |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added.
const String kMetric1 = 'flutter_repo_batch_maximum'; | ||
const String kMetric2 = 'flutter_repo_watch_maximum'; | ||
|
||
final MetricPoint cocoonPointRev1Metric1 = MetricPoint( |
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.
How a cocoon Point is related to skiaperf point and flutterengine point?
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.
Cocoon points (framework or device lab perf metrics) are not related to FlutterEnginePoint. Cocoon points should be able to convert to SkiaPerfPoint. I'll probably add more documentations when I migrate CocoonSource / BenchmarkMetricPoint in the future.
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.
LGTM
import 'package:metrics_center/src/common.dart'; | ||
|
||
/// Convenient class to capture the benchmarks in the Flutter engine repo. | ||
class FlutterEngineMetricPoint extends MetricPoint { |
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.
This pull request is not suitable for automatic merging in its current state.
|
This reverts commit a0ec4d6.
This reverts commit 2a5aa29.
Unit tests that cover their translations are also added.